-
Notifications
You must be signed in to change notification settings - Fork 30
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
Enable setting constraints on state variables #2340
Conversation
In addition to the current `Model.setStateIsNonNegative`, this adds the option to set additional (non)negativity/positivity constraints to be enforced by the solver. See [CVodeSetConstraints](https://sundials.readthedocs.io/en/latest/cvode/Usage/index.html#c.CVodeSetConstraints) for details. Related to AMICI-dev#2327.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2340 +/- ##
===========================================
+ Coverage 77.66% 77.70% +0.04%
===========================================
Files 324 324
Lines 20824 20864 +40
Branches 1454 1458 +4
===========================================
+ Hits 16173 16213 +40
Misses 4648 4648
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
python/tests/test_sbml_import.py
Outdated
assert np.any(rdata.x < 0) | ||
|
||
amici_solver.setRelativeTolerance(1e-14) | ||
amici_solver.setConstraints([1.0, 1.0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add an enum for 0.0
/1.0
/2.0
/-1.0
/-2.0
values for a bit more user convenience??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, probably makes sense to automatically add constraints when using Model::setStateIsNonNegative
and add this behaviour to the documentation of setConstraints and setStateIsNonNegative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add an enum for
0.0
/1.0
/2.0
/-1.0
/-2.0
values for a bit more user convenience??
done
also, probably makes sense to automatically add constraints when using
Model::setStateIsNonNegative
and add this behaviour to the documentation of setConstraints and setStateIsNonNegative.
I am not completely sure about this one. While I agree that it makes sense to imply the constraints in setStateIsNonNegative
True
, this would lead to some funny behavior when disabling non-negativity.
# I want to enable constraints:
model.setConstraints([non_negative])
# I want to enable the max(0, state) in addition:
model.setStateIsNonNegative([True])
# I decide to disable the latter
model.setStateIsNonNegative([False]) # implies setConstraints([none]) ?!
# now it also cleared my original constraint
I think I'd prefer to keep them independent for now.
In addition to the current
Model.setStateIsNonNegative
, this adds the option to set additional (non)negativity/positivity constraints to be enforced by the solver.See CVodeSetConstraints for details.
Related to #2327.