search

AMICI-dev / AMICI

Advanced Multilanguage Interface to CVODES and IDAS
https://amici.readthedocs.io/
Other
108 stars 31 forks source link

Split expressions into static and dynamic #2303

Closed dweindl closed 8 months ago

dweindl commented 8 months ago

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

codecov[bot] commented 8 months ago

Codecov Report

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

Project coverage is 77.83%. Comparing base (6a1cc3c) to head (0022111).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/graphs/tree.svg?width=650&height=150&src=pr&token=1bt9lbspzk&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev)](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `73.62% <38.07%> (+0.12%)` | :arrow_up: | | [cpp_python](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `34.35% <25.00%> (+0.23%)` | :arrow_up: | | [petab](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `36.90% <94.73%> (+0.33%)` | :arrow_up: | | [python](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `72.43% <38.07%> (+0.13%)` | :arrow_up: | | [sbmlsuite](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `∅ <ø> (∅)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | Coverage Δ | | |---|---|---| | [include/amici/abstract\_model.h](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-aW5jbHVkZS9hbWljaS9hYnN0cmFjdF9tb2RlbC5o) | `100.00% <ø> (ø)` | | | [include/amici/model.h](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-aW5jbHVkZS9hbWljaS9tb2RlbC5o) | `73.68% <ø> (ø)` | | | [models/model\_calvetti/dwdx.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2NhbHZldHRpL2R3ZHguY3Bw) | `100.00% <100.00%> (ø)` | | | [models/model\_calvetti/w.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2NhbHZldHRpL3cuY3Bw) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint/dwdp.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludC9kd2RwLmNwcA==) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint/dwdx.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludC9kd2R4LmNwcA==) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint/w.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludC93LmNwcA==) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint\_o2/dwdp.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludF9vMi9kd2RwLmNwcA==) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint\_o2/dwdx.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludF9vMi9kd2R4LmNwcA==) | `100.00% <100.00%> (ø)` | | | [models/model\_jakstat\_adjoint\_o2/w.cpp](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-bW9kZWxzL21vZGVsX2pha3N0YXRfYWRqb2ludF9vMi93LmNwcA==) | `100.00% <100.00%> (ø)` | | | ... and [35 more](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2303?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | |
dweindl commented 8 months ago

On second thought, I'll directly include dwd* here as well.

dweindl commented 8 months ago

Still something wrong here. The number of nonlinear convergence failures is significantly higher than before this change. :/

Resolved. Was related to other changes.

dweindl commented 8 months ago

For a model with nw: 227, nx: 67, ndwdx: 759, ndwdw: 471:

Looks like some further profiling/optimization is required...

dweindl commented 8 months ago

For a model with nw: 227, nx: 67, ndwdx: 759, ndwdw: 471:

* determining static expressions add 10 seconds to the previous 5 seconds for full code generation :(

* no observable difference in simulations times

Looks like some further profiling/optimization is required...

Okay, for that model, I am now down to no noticeable overhead and would merge those changes. 2.5 seconds overhead.

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.