-
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
Split expressions into static and dynamic #2303
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2303 +/- ##
===========================================
+ Coverage 77.72% 77.83% +0.10%
===========================================
Files 321 321
Lines 20601 20678 +77
Branches 1436 1440 +4
===========================================
+ Hits 16013 16095 +82
+ Misses 4585 4580 -5
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect. Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see AMICI-dev#2304) as amici expressions. Those static expressions will be handled more efficiently after AMICI-dev#2303. Related to AMICI-dev#2150.
2272e85
to
f2afbdf
Compare
Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect. Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see AMICI-dev#2304) as amici expressions. Those static expressions will be handled more efficiently after AMICI-dev#2303. Related to AMICI-dev#2150.
On second thought, I'll directly include dwd* here as well. |
71b08d6
to
1918faa
Compare
1918faa
to
19d1cd8
Compare
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.
👍 neat!
return self._static_indices[name] | ||
|
||
if name in ("dwdw", "dwdx", "dwdp"): | ||
static_indices_w = set(self.static_indices("w")) |
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.
how computationally intensive are those computations? Is caching worthwhile?
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.
Currently they are cached, but I didn't profile it yet.
|
||
# dynamic expressions | ||
if len(dynamic_idxs := self.model.dynamic_indices(function)): | ||
tmp_symbols = sp.Matrix( |
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.
partial duplication of code above
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.
True. But currently I don't think that moving those lines to some separate function will make it clearer or more maintainable, rather the opposite.
Resolved. Was related to other changes. |
Split expressions in `w` into dynamic (explicitly or implicitly time-dependent) and static ones. Evaluate static ones only when needed, i.e. after (re)initializing x_rdata or parameters. See AMICI-dev#1269
6cab686
to
f41894e
Compare
For a model with nw: 227, nx: 67, ndwdx: 759, ndwdw: 471:
Looks like some further profiling/optimization is required... |
Okay, for that model, I am now down to EDIT: I removed the derivative-based check for dwd* elements. With just checking for whether the expressions contain dynamic symbols, we have minimal overhead (<1s) and probably don't lose much. |
…2305) Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect. Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see #2304) as amici expressions. Those static expressions will be handled more efficiently after #2303. Related to #2150. See also #2304.
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.
👍
Split expressions in
w
and its derivatives into dynamic (explicitly or implicitly time-dependent) and static ones.Evaluate static ones only when needed, i.e. after (re)initializing x_rdata or parameters.
See #1269