-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: #455 Make IdentityLibrary a subclass of Polynomial Library #533
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
- Coverage 94.05% 94.04% -0.02%
==========================================
Files 38 37 -1
Lines 4157 4128 -29
==========================================
- Hits 3910 3882 -28
+ Misses 247 246 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Watcharin for grabbing another issue! Although it can't be moved to feature_library/base.py, can you move it to feature_library/polynomial_library.py? And also go through and delete tests/test parameterizations that no longer make sense?
Finally, #455 is an issue that had two options: subclass, or factory function. Can you describe why you chose subclass? I'm leaning slightly towards factory function, since we don't need all the infrastructure of a new class
To clarify, you want to delete all the test cases for IdentityLibrary as they are already covered in Polynomial?
I thought a class might work better with the typing system but I am not very familiar with Python typing system. |
Yes please
I can't think of any additional type safety provided by an Identity library, especially since the Identity Library is really unused. Let's make this a factory for |
#455 Make IdentityLibrary a subclass of Polynomial Library
Note that this class can't be moved to base because it imports from Polynomial Library which in turn imports from base and it will create dependencies issue.