Clean up MIOSR

[wesg52]

• Improve docstring
• Delete convenience code for experiments
• Add MIOSR to equality constraint tests

[Jacob-Stevens-Haas]

Hey @wesg52 , just want you to know we haven't forgotten about this. It's near the top of my priority list, and I think I should get to it by the weekend, if not later this week. Thanks for your patience.

[Jacob-Stevens-Haas]

Firstly, you write wonderfully. I really enjoyed reading the paper. It's exciting to see the opportunities that MIO provides for SINDy.

Secondly, can you close this PR and open one from wesg52:miosr directly into dynamicslab:master, rather than back into dynamicslab:miosr? I'm reading through the code now & preparing questions/comments but it's easier to add comments and review writ large that way. Sorry.

[wesg52]

First, thank you! I am glad you enjoyed and am excited to see people more knowledgable about dynamics find good use cases.

Second, I changed the target branch to dynamicslab:master, I assume that is sufficient.

Third, it seems like the free version of gurobipy worked fined in the CI test suite so that should hopefully not be an issue.

[wesg52]

Sorry for the delay. Thank you for the detailed look.

group_sparsity and target_sparsity are allowed together. However, I would expect all use cases where both are defined to have the property that target_sparsity < sum(group_sparsity) otherwise the target_sparsity constraint is redundant.

In general, target_sparsity || constraints => regress_jointly (technically there are some conditions where you could have per dimension constraints but I don't expect this to be an important use case.)

I believe I implemented all your suggestions and all tests are passing.

Regarding "how best to enable users to have fine grained control over the gurobi model..." I agree we don't need to implement this feature, given it is simple enough to just copy miosr and reimplement whatever modeling logic is required within make_model().

[codecov-commenter]

Codecov Report

Merging #219 (83fcc88) into master (cbe47de) will decrease coverage by 0.85%. The diff coverage is 76.28%.

:exclamation: Current head 83fcc88 differs from pull request most recent head 6462cda. Consider uploading reports for the commit 6462cda to get more accurate results

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   93.78%   92.92%   -0.86%     
==========================================
  Files          33       35       +2     
  Lines        3297     3507     +210     
==========================================
+ Hits         3092     3259     +167     
- Misses        205      248      +43     

Impacted Files	Coverage Δ
pysindy/__init__.py	91.66% <75.00%> (-3.58%)	:arrow_down:
pysindy/optimizers/__init__.py	86.66% <75.00%> (-13.34%)	:arrow_down:
pysindy/optimizers/miosr.py	76.40% <76.40%> (ø)
pysindy/feature_library/sindy_pi_library.py	74.81% <0.00%> (-20.43%)	:arrow_down:
pysindy/utils/base.py	83.73% <0.00%> (-6.04%)	:arrow_down:
pysindy/feature_library/pde_library.py	93.44% <0.00%> (-3.34%)	:arrow_down:
pysindy/feature_library/weak_pde_library.py	95.11% <0.00%> (-1.26%)	:arrow_down:
pysindy/optimizers/base.py	92.30% <0.00%> (-1.12%)	:arrow_down:
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[wesg52]

Alright, I think that does it! Let me know if there is anything else.

[akaptano]

Great job with this. I think the last thing that would be good to add is a simple example of the MIOSR functionality added to the "Optimizers" section of the Example 1 Jupyter notebook. There are some features of this algorithm that are unique, like the target_sparsity and group_sparsity, that would be good to demonstrate. And it would be good to add a Markdown cell above the section that describes why/when MIOSR is useful. Lastly, if we showcase it in Example 1, more people will see it!

[wesg52]

Thanks all!

I could imagine demonstrating

• target_sparsity vs. group_sparsity
• adding constraints
• running with verbose to print the gurobi output (and link docs explaining what it means) Anything else you think is especially important or anything from this list you would cut?

(Regardless, I will get to this Friday or Saturday.)

[akaptano]

This is a good list. My meager understanding is that MIOSR should beat ConstrainedSR3 on some problems, and this would be nice demonstration if you can cook up a dynamical system example where it does outperform ConstrainedSR3. I suppose this might require scanning all the MIOSR and ConstrainedSR3 hyperparameters to fully conclude though?

I think the most important things to show is that (1) we have now two algorithms in the code which can handle constraints (well three including trappingSR3 but it is derived from ConstrainedSR3) and (2) it is the only algorithm that can switch between these different notions of sparsity.

Thanks for your work on this!!

[wesg52]

Indeed, it would beat SR3 on some problems but I think for keeping with the simplicity and speed of the example notebook, I opted to just stay consistent with the other examples. Let me know if you'd like any changes to this!

[akaptano]

Looks great. I'm basically ready to approve and merge this, but last thing I was wondering is if we could begin the MIOSR section of the Example 1 notebook with something like an if statement that checks if gurobipy is installed (and if not, skips running this section), since the notebook will stop and give an error if the user hasn't done this.

[akaptano]

Hi Wes, do you mind fixing the conflicts with 1_feature_overview? You can use the options listed here to do that. I tried doing this and can successfully put everything together, but I do not have the gurobipy license, so when I rerun the example 1 notebook it gets rid of all the nice example output you have.

[akaptano]

Alternatively, just rerun the example script on the akaptano_miosr branch I just pushed.

[Jacob-Stevens-Haas]

It looks like the only conflicts are in the base64 encoded images and the conflict markers get put in stderr cells in the notebook json. Re-running the notebook clears them. (However, that might only work with certain diff drivers in git - I'm using nbdiff. Without that, git may add the <<<<, =====, >>>> markers in a way to break the json)

[wesg52]

Hi Alan, you automatically get the reduced license when you pip install gurobipy so I would suggest just doing that (and I imagine you will want it in the future to play with MIOSR anyway)

[akaptano]

I am finding that I have gurobipy, but it complains I do not have a license with a GurobiError. So "import gurobipy" works fine but then using it gives me a GurobiError saying I do not have a license. Not sure how to reconcile that with what you said... I think I downloaded gurobipy a long while ago though. I am using version 9.1.2. This might be just a problem on my end though. Let me know what y'all think and sorry this has been a slow PR to finish!

[wesg52]

Ah ok if you got it a while ago (e.g. when 9.1.2 came out) then your license has probably expired.

First, perhaps add a model within the try block (m = gp.Model()) which should fail if you don't have a valid license (as per this doc) so that the example notebook does not raise an exception in this case.

Second, update gurobi (I use somethings from the matrix interfaces in the 9.5 update so it would have failed even if you had a license) and 'renew' your license as described here.

[akaptano]

Closing this pull request and merging akaptano-miosr directly into the main branch. Thanks very much for your excellent work Wes and I'll reach out to Steve about having you present on an upcoming Friday, if you are still interested.