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

Trapping part 2/3 #493

Merged
merged 18 commits into from Apr 16, 2024
Merged

Trapping part 2/3 #493

merged 18 commits into from Apr 16, 2024

Conversation

Jacob-Stevens-Haas
Copy link
Collaborator

This is part 2/3, clarifying the way constraints are applied and allowing other constraints to be added to Trapping problems, by dispatching all to Constrainted SR3. It also solves some bugs in cvxpy probs. Part 3/3 will be the Mai's local trapping bit.

Currently there is a small hack - constraints are added at initialization, but in order to add trapping constraints, we need to know the size of the problem, which are data-dependent parameters. Solution in the interim is to add arguments to init that begin with an underscore.

This is much faster than fitting the features, and can be used
by trapping to replace some constraint index calculations.

Also:
Add docstring/type annotations
Rename a variable so it doesn't shadow newly-imported name
There are several major differences from the version in example 8:
* Coupled with PolynomialLibrary logic using PolynomialLibrary.powers_.
Previously, this coupling was implicit and actually tied to the specific order
in Example 8, not the order users would expect.
* Instead of handling pre-flattened 2D array, constraints are built with
target and feature axes separately, letting the caller handle formatting
the constraints as "target" or "feature".  This gives us flexibility to
later clean up the constraint logic outside this function without needing to
return here and removes the need for striding calculations like:
n_tgts * (n_tgts + k - 1) + i + n_tgts * int(j * (2 * n_tgts - j - 3) / 2.0)
Since constraints are built directly from the terms and exponents in the
library, its much easier to read and debug the code.
* Renamed "r" as "n_tgts", etc.  Single-letter index variables are
removed.  When they occur in homogenous iterators, they are concatenated with
what they're iterating (e.g. tgt_i, tgt_j)
* Broke out the different kinds of constraints into helper functions to make
it easier to test.

This allows follow on work to auto-attach constraints to a problem.
Previously, TrappingSR3 could only use constraints passed to it, and only then a
limited set of constraints.  It also didn't apply the trapping constraints
automatically, because constraints were required at __init__, and actually
shaping them requires knowledge about the number of features, typically not
known until fit (unless the user is a developer who knows how the feature
libraries work internally 😉)

WIP, Spawned issue #452
And simplify reorder_constraints
Read the fucking diff

backport-to: constraint-order
Also:
* (constrained_sr3)clean up creating cvxpy constraints
* (constrained_sr3)Clone the cvxpy problem in case of except statements,
because prob.solve() mutates prob in a mathematically significant way
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 95.79832% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 94.68%. Comparing base (80171b7) to head (cb1ab37).

Files Patch % Lines
pysindy/optimizers/constrained_sr3.py 92.59% 2 Missing ⚠️
pysindy/optimizers/trapping_sr3.py 96.82% 2 Missing ⚠️
pysindy/feature_library/polynomial_library.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   94.81%   94.68%   -0.14%     
==========================================
  Files          38       38              
  Lines        3996     4081      +85     
==========================================
+ Hits         3789     3864      +75     
- Misses        207      217      +10     

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

@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit 3413dc9 into master Apr 16, 2024
5 of 6 checks passed
@Jacob-Stevens-Haas Jacob-Stevens-Haas deleted the trapping-plumes branch April 16, 2024 20:41
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
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.

None yet

1 participant