Skip to content

Conversation

@yb6599
Copy link
Collaborator

@yb6599 yb6599 commented Oct 20, 2025

This PR separates DiscreteSINDy as a new class that derives from _BaseSINDy following the discussion in #351.
Also fixes #650.

@yb6599
Copy link
Collaborator Author

yb6599 commented Oct 20, 2025

The scikit-time tests fail as the scikit-time API has remained unchanged. How should I proceed with this? @Jacob-Stevens-Haas

@Jacob-Stevens-Haas
Copy link
Member

Jacob-Stevens-Haas commented Oct 20, 2025

Part of the problem with the face-to-face conversations is that I can't remember them.

Refresh my memory. What was the issue removing the parameter discrete_time from the scikit-time classes?

@yb6599
Copy link
Collaborator Author

yb6599 commented Oct 21, 2025

Removing discrete_time should not be a problem here. However, we had not decided how we would deal with the discrete time implementations of the scikit-time API.

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 95.80420% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.85%. Comparing base (472e5a2) to head (4fe2aa4).

Files with missing lines Patch % Lines
pysindy/_core.py 95.65% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
- Coverage   95.03%   94.85%   -0.18%     
==========================================
  Files          38       38              
  Lines        4106     4138      +32     
==========================================
+ Hits         3902     3925      +23     
- Misses        204      213       +9     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yb6599
Copy link
Collaborator Author

yb6599 commented Oct 21, 2025

Do we need _BaseSINDy.print() if we have SINDy.print() and DiscreteSINDy.print()?

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.

90% of the way there, thanks for this long work!

Few changes, few nits, few documentation/test things


# Compute derivatives with a finite difference method, for comparison
x_dot_test_computed = model.differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_differentiate is private - we can't show that in a tutorial.

Suggested change
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method(x_test, t=dt)


# Compute derivatives with a finite difference method, for comparison
x_dot_test_computed = model.differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method(x_test, t=dt)


# Compute derivatives with a finite difference method, for comparison
x_dot_test_computed = model.differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
x_dot_test_computed = model.differentiation_method._differentiate(x_test, t=dt)
x_dot_test_computed = model.differentiation_method(x_test, t=dt)

for i in range(1, n_steps):
x_train_map[i] = f(x_train_map[i - 1]) + eps * np.random.randn()
model = ps.SINDy(discrete_time=True)
model = ps.DiscreteSINDy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the two discrete examples from feature overview, keep them in the docstring for DiscreteSINDy.

Use the sphinx extension matplotlib.sphinxext.plot_directive to show the plots in the DiscreteSINDy documentation


feature_library: BaseFeatureLibrary
optimizer: _BaseOptimizer
discrete_time: bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

return self.optimizer.coef_

def equations(self, precision: int = 3) -> list[str]:
def equations(self, precision: int = 3, discrete_time=False) -> list[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Override this function, don't add parameters.

Comment on lines +1019 to +1033
if sys.version_info.minor < 10:
mixed_trajectories = (
isinstance(x, Sequence)
and (
not isinstance(x_dot, Sequence)
and x_dot is not None
or not isinstance(u, Sequence)
and u is not None
)
or isinstance(x_dot, Sequence)
and not isinstance(x, Sequence)
or isinstance(u, Sequence)
and not isinstance(x, Sequence)
)
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.9 reached EOL, require 3.10 in pyproject.toml and remove this condition

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.

[BUG] Process Trajectories does not consider derivatives if provided.

3 participants