Cached simplify in ODE export

[dilpath]

Decreases import time of a specific model with events by approximately 2.6x (n=1 ...).

[FFroehlich]

👍 neat

[codecov]

Codecov Report

Merging #1672 (8232cf8) into develop (9212bbd) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##           develop    #1672      +/-   ##
===========================================
- Coverage    78.23%   78.19%   -0.04%     
===========================================
  Files           70       70              
  Lines        11279    11286       +7     
===========================================
+ Hits          8824     8825       +1     
- Misses        2455     2461       +6     

Flag	Coverage Δ
cpp	75.24% <ø> (ø)
petab	62.22% <33.33%> (-0.09%)	⬇️
python	67.92% <33.33%> (-0.10%)	⬇️
sbmlsuite	87.67% <25.00%> (-0.34%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/amici/ode_export.py	92.96% <14.28%> (-0.65%)	⬇️
python/amici/pysb_import.py	93.90% <100.00%> (ø)
python/amici/sbml_import.py	96.10% <100.00%> (ø)

[dweindl]

If it's known to fail for pysb models, I think we shouldn't offer it as an option, but rather set it to False and add a comment that it isn't compatible.

[dilpath]

Hm, now not sure. Just tried to debug the PySB issue by settings the default value in pysb2amici and ode_model_from_pysb_importer to True, but all tests in pytest test_pysb.py passed.

[dweindl]

Also the PEtab pysb test suite? I am worried about the failure in e.g. https://github.com/AMICI-dev/AMICI/runs/5219969341?check_suite_focus=true. If caching silently leads to wrong model equations, this is a severe issue. Unless we have a good explanation of why that happened there and know that it won't happen again, I'd keep it out of there.

[dilpath]

After debugging with @dweindl , it seems to work for PySB (the tests in AMICI/python/tests/test_pysb.py and AMICI/tests/petab_test_suite/test_petab_suite.py pass).

However, it fails for PySB if deepcopy is in the cache method, with unknown causes. One reason for failure is due to incorrect gradients; with deepcopy, some AMICI C++ files related to gradients are not created. e.g. after running pytest --only-pysb --petab-cases 1 inside AMICI/tests/petab_test_suite, the diff for model files, with and without deepcopy, is:

Only in nocopy/build/temp.linux-x86_64-3.8: _model_dwdw_colptrs.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dwdw.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dwdw_rowvals.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dxdotdp_explicit_colptrs.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dxdotdp_explicit.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dxdotdp_explicit_rowvals.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_dydx.o
Only in nocopy/build/temp.linux-x86_64-3.8: _model_sx0.o
Files copy/build/temp.linux-x86_64-3.8/wrapfunctions.o and nocopy/build/temp.linux-x86_64-3.8/wrapfunctions.o differ
Files copy/CMakeLists.txt and nocopy/CMakeLists.txt differ
Files copy/_model/__model.cpython-38-x86_64-linux-gnu.so and nocopy/_model/__model.cpython-38-x86_64-linux-gnu.so differ
Files copy/_model/__pycache__/__init__.cpython-38.pyc and nocopy/_model/__pycache__/__init__.cpython-38.pyc differ
Only in nocopy: _model_dwdw_colptrs.cpp
Only in nocopy: _model_dwdw.cpp
Only in nocopy: _model_dwdw.h
Only in nocopy: _model_dwdw_rowvals.cpp
Only in nocopy: _model_dxdotdp_explicit_colptrs.cpp
Only in nocopy: _model_dxdotdp_explicit.cpp
Only in nocopy: _model_dxdotdp_explicit.h
Only in nocopy: _model_dxdotdp_explicit_rowvals.cpp
Only in nocopy: _model_dydx.cpp
Files copy/_model.h and nocopy/_model.h differ
Only in nocopy: _model_sx0.cpp

One possible cause of failure might be related to the PySB objects that exist inside ODEModel and are sent to cached_simplify, but this method expects SymPy objects.

Hence, this feature is turned off by default, for PySB models.

For SBML models, this caching can increase import times. For example, for a large model [1], this increased import times by approx. 10 mins, from 36 mins to 46 mins (some variability). However, no significant difference in RAM usage was seen. As the benefit of the caching appears to be model-specific, it is turned off by default for SBML models as well.

[1] https://github.com/ICB-DCM/CS_Signalling_ERBB_RAS_AKT/tree/master/FroehlichKes2018/PEtab

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

33.3% Coverage
0.0% Duplication