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

Implicit multiple trajectories #376

Merged
merged 7 commits into from
Aug 1, 2023
Merged

Conversation

Jacob-Stevens-Haas
Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas commented Jul 24, 2023

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
Copy link
Member Author

Jacob-Stevens-Haas commented Jul 25, 2023

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
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

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

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
@@            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 Coverage Δ
pysindy/pysindy.py 97.17% <100.00%> (+0.14%) ⬆️
pysindy/utils/base.py 85.62% <100.00%> (+1.89%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jacob-Stevens-Haas Jacob-Stevens-Haas linked an issue Jul 25, 2023 that may be closed by this pull request
@Jacob-Stevens-Haas
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member Author

Jacob-Stevens-Haas commented Jul 30, 2023

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.

Also:
    annotate and add docstring to validate_control_variables
    add AxesArray syntax
@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit 4aa3740 into master Aug 1, 2023
6 checks passed
@Jacob-Stevens-Haas Jacob-Stevens-Haas deleted the implicit-trajectories branch August 1, 2023 18:53
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
…ries

Make multiple trajectories implicit, treating a list of arrays for x, x_dot, or u as multiple trajectories.

Resolves dynamicslab#353
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.

Make multiple_trajectories implicit.
2 participants