dynamicslab / pysindy

A package for the sparse identification of nonlinear dynamical systems from data
https://pysindy.readthedocs.io/en/latest/
Other
1.46k stars 324 forks source link

ENH: #455 Make IdentityLibrary a subclass of Polynomial Library #533

Closed himkwtn closed 3 months ago

himkwtn commented 4 months ago

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.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

himkwtn commented 4 months ago

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 commented 3 months ago

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