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.
dweindl
force-pushed
the
feature_1269_split_exprs
branch
from
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. |
dweindl
force-pushed
the
feature_1269_split_exprs
branch
from
71b08d6
to
1918faa
Compare
dweindl
force-pushed
the
feature_1269_split_exprs
branch
from
1918faa
to
19d1cd8
Compare
| 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.
how computationally intensive are those computations? Is caching worthwhile?
There was a problem hiding this comment.
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.
partial duplication of code above
There was a problem hiding this comment.
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
dweindl
force-pushed
the
feature_1269_split_exprs
branch
from
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.
Split expressions in
wand 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