Skip to content

Use sbmlmath package for sympification of SBML math constructs#2681

Merged
dweindl merged 32 commits intoAMICI-dev:developfrom
dweindl:sbmlmath_gh-2146
Apr 23, 2025
Merged

Use sbmlmath package for sympification of SBML math constructs#2681
dweindl merged 32 commits intoAMICI-dev:developfrom
dweindl:sbmlmath_gh-2146

Conversation

@dweindl
Copy link
Copy Markdown
Member

@dweindl dweindl commented Apr 1, 2025

Directly parse the SBML MathML constructs without going through SBML L3 formula strings and sympy.sympify() due to various issues (#2146).

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 72.94118% with 23 lines in your changes missing coverage. Please review.

Project coverage is 77.75%. Comparing base (a313277) to head (ecd3462).

Files with missing lines Patch % Lines
python/sdist/amici/sbml_import.py 70.51% 23 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2681      +/-   ##
===========================================
- Coverage    77.83%   77.75%   -0.08%     
===========================================
  Files          332      332              
  Lines        23001    23031      +30     
  Branches      1480     1480              
===========================================
+ Hits         17902    17907       +5     
- Misses        5088     5113      +25     
  Partials        11       11              
Flag Coverage Δ
cpp 74.68% <77.02%> (-0.09%) ⬇️
cpp_python 33.58% <64.86%> (-0.03%) ⬇️
petab 39.42% <66.21%> (+<0.01%) ⬆️
python 72.79% <72.94%> (-0.08%) ⬇️

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

Files with missing lines Coverage Δ
python/sdist/amici/de_model.py 90.33% <100.00%> (+0.02%) ⬆️
python/sdist/amici/import_utils.py 82.97% <ø> (-6.39%) ⬇️
python/sdist/amici/sbml_utils.py 72.19% <ø> (-0.54%) ⬇️
python/sdist/amici/sbml_import.py 78.74% <70.51%> (-0.59%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dweindl added 5 commits April 1, 2025 18:51
Directly parse the SBML MathML constructs without going through SBML L3 formula strings are sympy.sympify due to various issues (AMICI-dev#2146).
@dweindl dweindl force-pushed the sbmlmath_gh-2146 branch from 1fd6c54 to cd55a97 Compare April 1, 2025 16:52
@dweindl dweindl force-pushed the sbmlmath_gh-2146 branch from 65fb47e to 2a635ff Compare April 2, 2025 12:18
@dweindl dweindl force-pushed the sbmlmath_gh-2146 branch from b24f490 to ac418ba Compare April 2, 2025 18:40
@dweindl dweindl self-assigned this Apr 2, 2025
@socket-security
Copy link
Copy Markdown

socket-security Bot commented Apr 2, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedsbmlmath@​0.4.0100100100100100

View full report

@dweindl dweindl marked this pull request as ready for review April 3, 2025 09:18
@dweindl dweindl requested a review from a team as a code owner April 3, 2025 09:18
f"{err}."
except (sp.SympifyError, TypeError, ZeroDivisionError) as err:
raise SBMLException(
f'{ele_name} "{var_or_math}" '
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will never be triggered from ele_name == "SymPy expression", right? Should the try claus also include "isinstance(var_or_math, sp.Basic)"?

Copy link
Copy Markdown
Member Author

@dweindl dweindl Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will never be triggered from ele_name == "SymPy expression", right?

right. that ele_name can be removed.

Should the try claus also include "isinstance(var_or_math, sp.Basic)"?

I don't think so. I don't recall errors during .subs()

),
)
try:
expr = self._mathml_parser.parse_str(mathml)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, make this one big try clause?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think .subs() will raise, I'd leave it out of there.

@dweindl dweindl added this pull request to the merge queue Apr 23, 2025
Merged via the queue into AMICI-dev:develop with commit f719e0f Apr 23, 2025
24 of 26 checks passed
@dweindl dweindl deleted the sbmlmath_gh-2146 branch April 23, 2025 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants