Sindyc

[briandesilva]

This pull request implements a SINDyC object (SINDy with control) #66 along with a full set of unit tests. The class is very similar to a standard SINDy object, but with an additional parameter, u, representing the control inputs, which is needed when the fit, predict, score, or simulate functions are called.

Instead of solving

X' = Theta(X) * Xi

SINDyC solves

X' = Theta(X, U) * Xi

where U is a matrix containing the control inputs.

Internally we essentially just ignore the control inputs when computing X', but use them when computing the library Theta(X, U).

This pull request also allows you to fit a model to a subset of the variables as in #40. You simply group all the variables in which you're interested inside X and the variables for which you aren't seeking differential equations inside U.

Other changes:

• Fix bugs in SINDy.process_multiple_trajectories causing x to be shrunk each time it was called with discrete_time=True and x_dot=None.

[codecov-io]

Codecov Report

Attention: Patch coverage is 96.11650% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.58%. Comparing base (bc13877) to head (2fc4106).
Report is 1462 commits behind head on master.

Files with missing lines	Patch %	Lines
pysindy/pysindy.py	94.80%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
- Coverage   95.67%   95.58%   -0.10%     
==========================================
  Files          18       18              
  Lines         810      884      +74     
==========================================
+ Hits          775      845      +70     
- Misses         35       39       +4     

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

[Ohjeah]

Overall the PR looks good to me. However, we have a lot of overlap with the main SINDy class except for the addition of control inputs. Is there a way we can generalize, e.g. base SINDy on SINDyC and leave u blank? Else we might run into a maintenance overhead for future changes.

[briandesilva]

Yes, it should be possible to combine them. The reason I didn't group them together from the beginning was just that it makes some of the important SINDy methods more cluttered (e.g. fit and simulate). I will take a stab at combining them and push the changes. If we decide it looks too sloppy we can always revert back to this version.

[briandesilva]

Okay I moved everything into one class at the cost of a few extra if-else blocks here and there. I also updated the unit tests to reflect the new structure.

On that topic, @Ohjeah, do you know of a good way to handle all the SINDYc unit tests at the same time as the SINDy ones? Most of the tests I wrote for SINDYc are very similar to the SINDy versions, but the SINDy tests require slightly different data fixtures than the SINDYc ones. E.g. for SINDy we might look like this

x, t = data
model = SINDy()
model.fit(x, t=t)
x_dot = model.predict(x)

assert x.shape == x_dot.shape

whereas the corresponding test for SINDYc would look like

x, t, u, _ = data
model = SINDy()
model.fit(x, u=u, t=t)
x_dot = model.predict(x, u=u)

assert x.shape == x_dot.shape

Do you have any ideas on how to combine tests like these?

[Ohjeah]

Usually I think it is a good idea to be a little bit more verbose with test such that the unit tests are easily understandable and do not have additional logic.

You could parametrize like this though:

@pytest.mark.parametrize()
def test_shape(model_cls, data):
    model = model_cls()
    *inputs, time = data

    model.fit(*inputs, t=t)
    x_dot = model.predict(*inputs)
    assert inputs[0].shape == x_dot.shape

[briandesilva]

I will leave them as separate tests for now for the sake of readability.

[Ohjeah]

Sure.

The complexity test is still failing sporadically. We should try to constrain that a bit more and make it more deterministic. Should I accept the PR anyway?

[briandesilva]

Sorry, I didn't see your comment before making the most recent commit. I relaxed the complexity test constraint a bit more.

I was able to reproduce the failed test locally by using the same parameters: n_samples=1124, n_features=27, n_informative=5, random_state=6. It looks like LASSO somehow finds 4 informative features, while the other heavily regularized models find 6. This seems like a bit of a fluke; for different random states this doesn't occur.

Maybe we could remove LASSO from this test? Or we could stick with the quick fix I made.

In any case, the bug isn't caused by this PR, so I think it's okay to accept it.