Skip to content
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

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

himkwtn
Copy link
Collaborator

@himkwtn himkwtn commented Jul 16, 2024

#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.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.04%. Comparing base (3503c15) to head (ec05488).
Report is 14 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas left a 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

@himkwtn
Copy link
Collaborator Author

himkwtn commented Jul 25, 2024

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?

To clarify, you want to delete all the test cases for IdentityLibrary as they are already covered in Polynomial?

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

I thought a class might work better with the typing system but I am not very familiar with Python typing system.

@Jacob-Stevens-Haas
Copy link
Member

To clarify, you want to delete all the test cases for IdentityLibrary as they are already covered in Polynomial?

Yes please

I thought a class might work better with the typing system but I am not very familiar with Python typing system.

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 PolynomialLibrary

test/test_feature_library.py Outdated Show resolved Hide resolved
test/test_feature_library.py Outdated Show resolved Hide resolved
test/test_feature_library.py Show resolved Hide resolved
@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit 476b59f into master Jul 30, 2024
5 of 7 checks passed
@Jacob-Stevens-Haas Jacob-Stevens-Haas deleted the enh-455-identity-library branch July 30, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants