Piecewise to heaviside by paulstapor · Pull Request #1352 · AMICI-dev/AMICI

paulstapor · 2020-12-02T16:07:06Z

Piecewise functions were so far parsed into if-else-statements, which made the model files poorly readable. Much more important though, the problem was that there might have been a discontinuity in the right hand side, but the solver didn't notice this and ran over it. This could lead to poor accuracy of the solver.

However, back from Matlab times, AMICI had it's own interface for Heaviside functions. If a piecewise function gets translated into an AMICI-Heaviside function (using the existing C++ code), both problems are solved. Moreover, this implementation should be a first step into the direction of implementing evens on the Python side.

I know this is a comparably large pull-request, sorry. But there's no way to split this into smaller parts, except including substantially less functionality, which is however urgently needed...

This implementation tries to stay as close to the existing event implementation as possible. It is supposed to be working for forward sensitivities and Heaviside functions in full generality, i.e., also supporting parameter and state-dependent arguments of HEaviside functions.

…e h is known when dxdotdx_explicit is generated, to have the header file of h included

… theorem

codecov · 2020-12-02T16:20:26Z

Codecov Report

Merging #1352 (d9d1bfe) into develop (24b9b98) will increase coverage by 8.94%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop    #1352      +/-   ##
===========================================
+ Coverage    78.44%   87.38%   +8.94%     
===========================================
  Files           62        2      -60     
  Lines         9797     1427    -8370     
===========================================
- Hits          7685     1247    -6438     
+ Misses        2112      180    -1932

Flag	Coverage Δ
cpp	`?`
petab	`?`
python	`?`
sbmlsuite	`87.38% <75.00%> (-1.23%)`	⬇️

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

Impacted Files	Coverage Δ
python/amici/ode_export.py	`85.51% <61.72%> (-5.57%)`	⬇️
python/amici/sbml_import.py	`90.13% <97.87%> (-6.32%)`	⬇️
python/amici/parameter_mapping.py
ThirdParty/gsl/gsl/gsl-lite.hpp
src/model_ode.cpp
python/amici/petab_import.py
python/amici/custom_commands.py
include/amici/newton_solver.h
... and 50 more

python/amici/ode_export.py

python/amici/sbml_import.py

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

paulstapor · 2020-12-02T21:08:41Z

Thanks for the code review! I'll implement the suggestions as soon as the test cases are there!

python/amici/ode_export.py

python/amici/sbml_import.py

…/AMICI into piecewise_to_heaviside

python/amici/ode_export.py

FFroehlich

Good to go from my end

dweindl · 2020-12-09T18:58:23Z

documentation/python_interface.rst

 ++++++++++++++++++++++++++++++++++++++

-Python-AMICI currently **passes 850 out of the 1780 (~48%) test cases** from
+Python-AMICI currently **passes 805 out of the 1780 (~45%) test cases** from


Not sure what happened, looking at the tests that are skipped I couldn't really identify those 45 that are not longer passing.

Ah, this is unrelated to this PR. (Here it increases from 804 to 805). This is where it happened: https://github.com/AMICI-dev/AMICI/actions/runs/331448378

Used to work but skipped now (Should be all those AMICI does not support SBML models containing variables with SId):

969 970 971 974 1104 1105 1107 1109 1121 1434 1436 1438 1440 1442 1465 1498 1552 1554 1574 1638 1639 1651 1652 1653 1654 1655 1656 1657 1722 1724 1725 1726 1733 1734 1735 1744 1746 1748 1750 1753 1764 1765 1766 1767 1768 1773 1774

I suspect thats due to the skippings with msgs like

SKIPPED [2] tests/testSBMLSuite.py:87: AMICI does not support SBML models containing variables with SId k0. 3677 SKIPPED [3] tests/testSBMLSuite.py:87: AMICI does not support SBML models containing variables with SId A1_sr. 3678 SKIPPED [2] tests/testSBMLSuite.py:87: AMICI does not support SBML models containing variables with SId A_sr1.

Those are a little over 50 cases, so lets have a more detailed look in a different PR. Also looks like there are still a handful of low hanging fruits like ceil/min/max applied to numerical values, which we should be able to pass.

Yes, separate PR.

python/amici/ode_export.py

python/amici/sbml_import.py

Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>

…/AMICI into piecewise_to_heaviside

paulstapor · 2020-12-09T20:12:50Z

Just also had a look at the refactored test. Looks fine for me as well!

sonarqubecloud · 2020-12-09T22:09:44Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

33.9% Coverage
0.5% Duplication

* Fix: Don't require code context for call stack (fixes #1347) (#1348) * Fix Intel compiler warnings (#1349) 'type qualifier on return type is meaningless' Closes #1346 * Avoid segfault in CVODES CVAdataStore (#1351) Does not fix the root cause of occassional segfaults, but turns them into exceptions / integration failues. * CI: Move windows tests from TravisCI to GitHub Actions (#1354) Closes #1350 Removes obsolete .travis.yml * Fix cxxcode import for SymPy 1.6 (#1356) * Piecewise to heaviside (#1352) * parse piecewise into heaviside functions * Add Events as model quantity, parse Heavisides slightly differently * bugfix * rename symbols of root to h and switch order of functions to make sure h is known when dxdotdx_explicit is generated, to have the header file of h included * rename root symbols to HeavisideFunction_ to have things more indicative * tell AMICI about root, stau, and deltasx functions * heavisides without sensitivities should be working * need to change generation of stau to generration via inverse function theorem * compute stau via inverse function theorem * added functionality for deltasx, but segfaulting * bugfixing * remove debug output * get indentations right * Update python/amici/ode_export.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * Update python/amici/ode_export.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * Update python/amici/ode_export.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * Update python/amici/ode_export.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * Update python/amici/ode_export.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * Update python/amici/sbml_import.py Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * add a test for trajectories * fix sroot and test * cosmetic * add analytical expressions for sensitivities * add test for a complex condition * add test for many conditions * fix test * improve documentation * rename tests and refactor test for iterated piecewise, to check SBML piecewise logic a bit further * cleanup for code review * further cleanup * quarter way done with parsing heavisides into piecewise * fix logic parsing * monkeypatch sympy printer * enable more tests * implement denesting of piecewise * fix SBML nonsense * fix boolean triggers * simplify logic checks * fix code smells * add events to variable prototypes * cleanup * report details for skipped tests durin GHA run * better skip printing * resubstitution of non-optimized RHS into stau * add sensitivity tests * refactor tests * remove skipping display code * update test stats * Update python/amici/ode_export.py * Apply suggestions from code review Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com> * document and simplify _collect_heaviside_roots * address review comments * reduce complexity * misplaced docstrings Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> Co-authored-by: dilpath <dilan.private+github@outlook.com> Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com> * restore support for species references (#1358) * restore support for species references * fixup and improve error message * extend support for sbml functions (#1357) * add more function mappings * fixup min/max * fix rem * add Mod to unsupported functions * more trigonometric functions * more functions, fix xref * fix xref with init assignment * extend unsupported functions * fix plus with empty arguments, update statistics * update test stats * fix stats * Refactor: introduce import_utils.py (#1361) * Refactor: introduce import_utils.py To collect format-independent functions for model import. * add additional file to SBML Testsuite path check * Include new module in doc Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * CI: Show all compiler warnings and treat as error (#1362) * CI: Show all compiler errors and treat as error Closes #1167 * Fix unused arguments * Fix deprecated-copy * Fix NVector (non-)const mess * Fix CMake deprecation warning * Fix doxygen * Fix some typos * Simplify / reduce number of const_casts * Add support for different noise models for PySB import (#1360) * Add support for different noise models for PySB import .. as already the case for SBML import. Closes #1176 * Respect noise model selection for PySB-PEtab import (#1339) * doc, import * Respect observable transformation for PySB-PEtab import (Fixes #1339) * Set ODEModel._has_quadratic_nllh Co-authored-by: Fabian Fröhlich <fabian@schaluck.com> * version bump * fix some code smells to trigger GHA builds again? * update docker to 20.04 * pin breathe Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com> Co-authored-by: Lorenzo Contento <lcontento@users.noreply.github.com> Co-authored-by: Paul Stapor <paul.stapor@helmholtz-muenchen.de> Co-authored-by: dilpath <dilan.private+github@outlook.com>

paulstapor added 13 commits November 30, 2020 13:43

parse piecewise into heaviside functions

c46d57d

Add Events as model quantity, parse Heavisides slightly differently

f27004f

bugfix

c2e7543

rename symbols of root to h and switch order of functions to make sur…

4ffa4ca

…e h is known when dxdotdx_explicit is generated, to have the header file of h included

rename root symbols to HeavisideFunction_ to have things more indicative

0382baf

tell AMICI about root, stau, and deltasx functions

7a121a5

heavisides without sensitivities should be working

5b9fc4e

need to change generation of stau to generration via inverse function…

d1cbf26

… theorem

compute stau via inverse function theorem

3bca674

added functionality for deltasx, but segfaulting

d736d36

bugfixing

fbf0775

remove debug output

a7fc833

get indentations right

224e10e

paulstapor requested review from FFroehlich and dilpath December 2, 2020 16:07

Merge branch 'develop' into piecewise_to_heaviside

e7583fb

FFroehlich requested changes Dec 2, 2020

View reviewed changes

FFroehlich reviewed Dec 2, 2020

View reviewed changes

paulstapor and others added 6 commits December 2, 2020 22:01

Update python/amici/ode_export.py

dbd245a

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

Update python/amici/ode_export.py

f25f2c9

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

Update python/amici/ode_export.py

5272337

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

Update python/amici/ode_export.py

1ba9942

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

Update python/amici/ode_export.py

ca8879f

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

Update python/amici/sbml_import.py

5d1a3cc

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

dweindl reviewed Dec 3, 2020

View reviewed changes

dilpath and others added 3 commits December 4, 2020 02:36

add a test for trajectories

8405b98

fix sroot and test

762a8fd

cosmetic

26f8f68

FFroehlich and others added 10 commits December 8, 2020 23:17

add events to variable prototypes

c6c0f7c

cleanup

1c6456f

report details for skipped tests durin GHA run

ff2a4bb

better skip printing

a2e4967

resubstitution of non-optimized RHS into stau

d40f8ba

add sensitivity tests

b05cc7d

refactor tests

209f992

remove skipping display code

983823e

Merge branch 'piecewise_to_heaviside' of https://github.com/AMICI-dev…

a1c7f6b

…/AMICI into piecewise_to_heaviside

update test stats

748b1eb

FFroehlich reviewed Dec 9, 2020

View reviewed changes

python/amici/ode_export.py Outdated Show resolved Hide resolved

FFroehlich approved these changes Dec 9, 2020

View reviewed changes

FFroehlich requested a review from dweindl December 9, 2020 17:06

Update python/amici/ode_export.py

c48b8a4

dweindl approved these changes Dec 9, 2020

View reviewed changes

FFroehlich and others added 4 commits December 9, 2020 14:38

Apply suggestions from code review

5d897fe

Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>

document and simplify _collect_heaviside_roots

eb64393

Merge branch 'piecewise_to_heaviside' of https://github.com/AMICI-dev…

46805ab

…/AMICI into piecewise_to_heaviside

address review comments

61a36b6

FFroehlich and others added 2 commits December 9, 2020 16:01

reduce complexity

3f81ad1

misplaced docstrings

33eb0ec

FFroehlich merged commit 4010909 into develop Dec 10, 2020

FFroehlich deleted the piecewise_to_heaviside branch December 10, 2020 00:35

dweindl linked an issue Dec 10, 2020 that may be closed by this pull request

Incomplete implementation of Piecewise #1317

Closed

FFroehlich mentioned this pull request Dec 12, 2020

Release 0.11.11 #1363

Merged

FFroehlich mentioned this pull request Dec 13, 2023

Fix piecewise/Heaviside handling #2234

Merged

dweindl mentioned this pull request Oct 17, 2024

SBML import: Incorrect processing of piecewise/Heaviside function #2545

Closed

Conversation

paulstapor commented Dec 2, 2020

Uh oh!

codecov bot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulstapor commented Dec 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FFroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

dweindl Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

FFroehlich Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

dweindl Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

dweindl Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FFroehlich Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

dweindl Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulstapor commented Dec 9, 2020

Uh oh!

sonarqubecloud bot commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 2, 2020 •

edited

Loading

dweindl Dec 9, 2020 •

edited

Loading