dynamicslab / pysindy

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

Implicit multiple trajectories #376

Closed Jacob-Stevens-Haas closed 1 year ago

Jacob-Stevens-Haas commented 1 year ago

For your consideration, here's a PR that implements #353.

tl;dr: IMO the multiple_trajectories argument is unneccessary and its removal simplifies calling signatures. SINDy.fit() knows users want multiple trajectories when they pass a list of arrays - the same way many numpy functions have implicit behavior when passed a list of arrays instead of a single array. OTOH, there exists a niche case where someone calls fit() with x as a list instead of an array and t as a float. Previously, this would give an error, while in this PR, it would be allowed, similar to how submitting transposed data is allowed but doesn't give people what they think. If people desire, I can add guard code around specifically this case, but my feeling is that the docstring is clear and the edge case may not exist in the wild.

Jacob-Stevens-Haas commented 1 year ago

So this passes in 3.10, but fails <3.10 because isinstance didn't used to work for subscripted generics... working on a fix.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10% :tada:

Comparison is base (044b4f5) 93.70% compared to head (fe3e4ad) 93.81%. Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #376 +/- ## ========================================== + Coverage 93.70% 93.81% +0.10% ========================================== Files 37 37 Lines 3589 3604 +15 ========================================== + Hits 3363 3381 +18 + Misses 226 223 -3 ``` | [Files Changed](https://app.codecov.io/gh/dynamicslab/pysindy/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dynamicslab) | Coverage Δ | | |---|---|---| | [pysindy/pysindy.py](https://app.codecov.io/gh/dynamicslab/pysindy/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dynamicslab#diff-cHlzaW5keS9weXNpbmR5LnB5) | `97.17% <100.00%> (+0.14%)` | :arrow_up: | | [pysindy/utils/base.py](https://app.codecov.io/gh/dynamicslab/pysindy/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dynamicslab#diff-cHlzaW5keS91dGlscy9iYXNlLnB5) | `85.62% <100.00%> (+1.89%)` | :arrow_up: |

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

Jacob-Stevens-Haas commented 1 year ago

This counts as green CI, I believe. Similar situation as before - deletion PR with 100% of diff covered, but because covered lines were deleted, overall coverage drops.

znicolaou commented 1 year ago

I'm good with this change! Only request: run each of the example notebooks to verify that the edge case doesn't need to be eliminated in the notebooks.

Jacob-Stevens-Haas commented 1 year ago

Thanks @znicolaou! I searched through all the notebooks, and anything that used discrete_time=True made sure to cast each trajectory as a numpy array before fit(). But since master is tracking a 2.0 release, most of the notebooks fail for other breaks in backwards compatibility during the last few PRs.

My short fix for the notebooks (later; as we get closer to 2.0) will be to add an assert pysindy.__version__ < 2 to the first cell of any notebook that isn't in a fast-testable format. Right now that's everything other than 1, 2, and 5. Long term, we should talk about how to revamp examples/documentation, e.g. whether to pin examples to specific versions of pysindy. I feel like it's reasonable, if API reference is correct, to expect a user to be able to update any code based upon a research example notebook pinned to an old version. e.g. ensembling.