Smith benchmark and SBML initialization fix #2034
Conversation
python/sdist/amici/petab_import.py
Outdated
| compartment = sbml_model.getCompartment( | ||
| sbml_model.getSpecies(assignee_id).getCompartment() | ||
| ).getId() | ||
| formula = f'({formula}) / {compartment}' |
There was a problem hiding this comment.
Hm, somehow I thought species with hasOnlySubstanceUnits=True should have amounts here, not concentrations.
There was a problem hiding this comment.
I was also a bit confused, but the initial assignments processing code in amici will assume the formula yields a concentration which then needs to be converted to an amount to assign the species.
There was a problem hiding this comment.
Hm, but doesn't that go against the SBML spec?
SBML L3V2 section 4.8.5:
• In the case of a species, an InitialAssignment sets the referenced species’ initial quantity (concentration or
amount) to the value determined by the formula in math. The unit associated with the value produced by
the math formula should be equal to the unit associated with the species’ quantity. (See Section 4.6.5 on
p. 51 for an explanation of how a species’ quantity is determined.)
4.6.5
The interpretation is controlled by
the attribute hasOnlySubstanceUnits. If the attribute has the value “false”, then the unit of measurement
associated with the value of the species is {unit of amount}/{unit of size} (i.e., concentration or density). If
hasOnlySubstanceUnits has the value “true”, then the value is interpreted as having a unit of amount only.
There was a problem hiding this comment.
hmm, that sounds like our approach to handling these assignment rules is wrong then. Suprising that the sbml testsuite passes nevertheless.
There was a problem hiding this comment.
Suprising that the sbml testsuite passes nevertheless.
Indeed.
There was a problem hiding this comment.
Suprising that the sbml testsuite passes nevertheless.
Indeed.
okay fixed in sbml_import. Looks like not a single past fails 🤔
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2034 +/- ##
===========================================
- Coverage 75.27% 75.26% -0.01%
===========================================
Files 75 75
Lines 13017 13015 -2
===========================================
- Hits 9798 9796 -2
Misses 3219 3219
Flags with carried forward coverage won't be shown. Click here to find out more.
|
dweindl
changed the title
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
requires Benchmarking-Initiative/Benchmark-Models-PEtab#180