yaml2sbml-dev / yaml2sbml

Tool to convert an ODE model specified in the YAML format to SBML.
https://yaml2sbml.readthedocs.io/en/latest/
Other
12 stars 7 forks source link

Avoid unit consistency checks #135

Closed dilpath closed 3 years ago

dilpath commented 3 years ago

Units are always dimensionless in the generated SBML models https://github.com/yaml2sbml-dev/yaml2sbml/blob/6f42ee593796765605f2735066e157c7b1352861/yaml2sbml/yaml2sbml.py#L288 https://github.com/yaml2sbml-dev/yaml2sbml/blob/6f42ee593796765605f2735066e157c7b1352861/yaml2sbml/yaml2sbml.py#L328 https://github.com/yaml2sbml-dev/yaml2sbml/blob/6f42ee593796765605f2735066e157c7b1352861/yaml2sbml/yaml2sbml.py#L446

This can lead to inconsistent units. With the following YAML, the SBML checkConsistency results in a segmentation fault (python-libsbml == 5.19.0).

assignments:
- assignmentId: Pprp
  formula: pow(10, log10(GlucH))
odes:
- initialValue: GlucH0
  rightHandSide: GlucH
  stateId: GlucH
parameters:
- nominalValue: 1
  parameterId: GlucH0

Units have no effect in SBML so these false inconsistencies can be ignored.

codecov-commenter commented 3 years ago

Codecov Report

Merging #135 (8a312bc) into develop (3c45897) will increase coverage by 9.74%. The diff coverage is 86.27%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #135      +/-   ##
===========================================
+ Coverage    77.85%   87.60%   +9.74%     
===========================================
  Files            5        6       +1     
  Lines          429      476      +47     
===========================================
+ Hits           334      417      +83     
+ Misses          95       59      -36     
Impacted Files Coverage Δ
yaml2sbml/YamlModel.py 84.00% <77.77%> (-0.49%) :arrow_down:
yaml2sbml/yaml_validation.py 89.65% <77.77%> (+17.65%) :arrow_up:
yaml2sbml/yaml2sbml.py 86.50% <84.37%> (+11.11%) :arrow_up:
yaml2sbml/__init__.py 100.00% <100.00%> (ø)
yaml2sbml/version.py 100.00% <100.00%> (ø)
yaml2sbml/yaml2PEtab.py 94.17% <100.00%> (+23.46%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 773f4c5...8a312bc. Read the comment docs.

jvanhoefer commented 3 years ago

Hey @dilpath & @yannikschaelte, Thank you for pointing this out! I would suggest instead of removing the SBML checks, to not assign Units in the SBML in the first place, as the SBML checks can be useful. For example to check, if every parameter appearing in the right hand side of the ODEs is defined in the parameter section etc. ...

dilpath commented 3 years ago

Hey @dilpath & @yannikschaelte, Thank you for pointing this out! I would suggest instead of removing the SBML checks, to not assign Units in the SBML in the first place, as the SBML checks can be useful. For example to check, if every parameter appearing in the right hand side of the ODEs is defined in the parameter section etc. ...

This PR only disables unit checks, but other SBML checks should still be enabled.

jvanhoefer commented 3 years ago

And one more problem with this approach: If you remove the SBML validity checks, your .yaml model will still result in an invalid SBML (and your simulator should complain about that later on...). Hence I would go for outputting a valid SBML instead :)

jvanhoefer commented 3 years ago

@dilpath thank you for pointing me, that I overlooked, that the other tests are still in place. Hence I am fine with merging :) Sorry for the confusion :)