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

Remove unneccessary/dead code #347

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Remove unneccessary/dead code #347

merged 3 commits into from
Jun 28, 2023

Conversation

Jacob-Stevens-Haas
Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas commented Jun 20, 2023

  • We now require scikitlearn>=1.1, so no need to guard compatibility with <1.0
  • the quiet parameter in SINDy.fit() isn't our business. Client code should have responsibility for handling warnings. Keeps fit() simpler for subclassing.
  • removed three other parameters that had no functionality.

Technically this is backwards incompatible, but since master is tracking a 2.0 release, that should be ok.

  • There were some cases where example 1 used the quiet parameter. This motivated refactoring example 1 to the testable format, which also allows us to remove papermill from the CI (pytest tests the notebook, and faster).

We no longer allow scikit-learn<1.0, so we can remove compatibility.  Lots of
wet code.
Three of these parameters do nothing.
"quiet" is unneccessary - user code should silence.
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.02 🎉

Comparison is base (5f224ab) 91.92% compared to head (48d076f) 93.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   91.92%   93.95%   +2.02%     
==========================================
  Files          37       37              
  Lines        3764     3688      -76     
==========================================
+ Hits         3460     3465       +5     
+ Misses        304      223      -81     
Impacted Files Coverage Δ
pysindy/feature_library/parameterized_library.py 100.00% <ø> (ø)
pysindy/deeptime/deeptime.py 100.00% <100.00%> (+3.03%) ⬆️
pysindy/feature_library/base.py 95.91% <100.00%> (+1.39%) ⬆️
pysindy/feature_library/custom_library.py 100.00% <100.00%> (+3.79%) ⬆️
pysindy/feature_library/fourier_library.py 100.00% <100.00%> (+4.28%) ⬆️
pysindy/feature_library/generalized_library.py 97.34% <100.00%> (+4.97%) ⬆️
pysindy/feature_library/identity_library.py 96.96% <100.00%> (+6.96%) ⬆️
pysindy/feature_library/pde_library.py 95.53% <100.00%> (+2.44%) ⬆️
pysindy/feature_library/polynomial_library.py 98.05% <100.00%> (+3.46%) ⬆️
pysindy/feature_library/sindy_pi_library.py 95.31% <100.00%> (+20.49%) ⬆️
... and 2 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Not tested by default from command line, but explicitly enabled in CI
Removed papermill from CI
@Jacob-Stevens-Haas
Copy link
Member Author

Hey guys I'm giving y'all the heads up about this, but it's pretty minor and if you'd rather not review it, I'll merge it in a few days.

@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit ac61b4a into master Jun 28, 2023
@Jacob-Stevens-Haas Jacob-Stevens-Haas deleted the quick-fixes branch June 28, 2023 19:48
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
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.

1 participant