search

AMICI-dev / AMICI

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

Fix petab import: initial assignment targets as constant parameters #2345

Closed dweindl closed 6 months ago

dweindl commented 6 months ago

During PEtab import, parameters that are targets of initial assignments have so far not been turned into constant parameters, because they didn't exist in the amici model (see #2304). Now that those parameters remain in the model, they should be turned into constant parameters, unless specified otherwise.

Requires https://github.com/PEtab-dev/libpetab-python/pull/248, otherwise PEtab parameter mapping will provide potentially incorrect values for those parameters.

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 77.64%. Comparing base (19f7baf) to head (c0092c7). Report is 3 commits behind head on develop.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/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/2345?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 #2345 +/- ## =========================================== + Coverage 77.63% 77.64% +0.01% =========================================== Files 324 324 Lines 20769 20786 +17 Branches 1449 1449 =========================================== + Hits 16124 16140 +16 - Misses 4642 4643 +1 Partials 3 3 ``` | [Flag](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/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/2345/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `73.40% <23.07%> (-0.03%)` | :arrow_down: | | [cpp_python](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `34.27% <46.15%> (+<0.01%)` | :arrow_up: | | [petab](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `36.90% <95.23%> (+0.15%)` | :arrow_up: | | [python](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | `72.21% <46.15%> (-0.02%)` | :arrow_down: | | [sbmlsuite](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345/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/2345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev) | Coverage Δ | | |---|---|---| | [python/sdist/amici/sbml\_import.py](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-cHl0aG9uL3NkaXN0L2FtaWNpL3NibWxfaW1wb3J0LnB5) | `78.63% <100.00%> (+0.04%)` | :arrow_up: | | [python/tests/test\_petab\_import.py](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-cHl0aG9uL3Rlc3RzL3Rlc3RfcGV0YWJfaW1wb3J0LnB5) | `100.00% <100.00%> (ø)` | | | [python/sdist/amici/petab/sbml\_import.py](https://app.codecov.io/gh/AMICI-dev/AMICI/pull/2345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AMICI-dev#diff-cHl0aG9uL3NkaXN0L2FtaWNpL3BldGFiL3NibWxfaW1wb3J0LnB5) | `54.92% <90.00%> (+4.92%)` | :arrow_up: |
FFroehlich commented 6 months ago

On second thought, maybe add the libsbml parser settings as module variable and import rather than duplicating?

dweindl commented 6 months ago

On second thought, maybe add the libsbml parser settings as module variable and import rather than duplicating?

Would like to, but it takes a specific model as first argument. The best thing I could do, would be adding a function that creates a parser, I think, to not duplicate the settings.