Skip to content

Conversation

@dweindl
Copy link
Member

@dweindl dweindl commented Nov 1, 2025

Distinguish between id: str and symbol: sp.Symbol. The old behavior (def get_id(self) -> sp.Symbol) was confusing.

Fix inconsistent type for AlgebraicEquation symbol/id.

Closes #2940.

@dweindl dweindl self-assigned this Nov 1, 2025
@codecov
Copy link

codecov bot commented Nov 1, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.03%. Comparing base (3ada40c) to head (861a135).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
python/sdist/amici/de_model_components.py 91.89% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3001      +/-   ##
==========================================
- Coverage   76.70%   76.03%   -0.68%     
==========================================
  Files         309      309              
  Lines       19891    19893       +2     
  Branches     1498     1498              
==========================================
- Hits        15258    15125     -133     
- Misses       4620     4755     +135     
  Partials       13       13              
Flag Coverage Δ
cpp 72.53% <91.66%> (+<0.01%) ⬆️
cpp_python 37.88% <79.16%> (+<0.01%) ⬆️
petab 38.58% <79.16%> (+<0.01%) ⬆️
petab_sciml 14.15% <58.33%> (+0.01%) ⬆️
python 69.35% <93.75%> (+<0.01%) ⬆️
sbmlsuite-jax ?

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 92.12% <100.00%> (-0.39%) ⬇️
python/sdist/amici/pysb_import.py 94.47% <100.00%> (ø)
python/sdist/amici/sbml_import.py 81.45% <100.00%> (-12.50%) ⬇️
python/sdist/amici/de_model_components.py 91.37% <91.89%> (-1.65%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dweindl dweindl force-pushed the gh-2940-id-vs-sym branch 2 times, most recently from 95c0fcc to 209fc78 Compare November 1, 2025 13:25
@dweindl dweindl marked this pull request as ready for review November 1, 2025 14:27
@dweindl dweindl requested a review from a team as a code owner November 1, 2025 14:27
identifier of the ModelQuantity
"""
return self._identifier
return (
Copy link
Member

Choose a reason for hiding this comment

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

This looks more like a get_name than a get_id to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

The amici IDs correspond to sympy names (Symbol.name). The AMICI names can be different and don't have to be unique. get_name is just below.

Copy link
Member

@FFroehlich FFroehlich Nov 1, 2025

Choose a reason for hiding this comment

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

Ah I see, that sounds like we can replace most if not all strip_pysb(c.get_symbol) and str(c.get_sym()) with c.get_id() then and get rid of strip_pysb.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be great. I was about to ask whether we can get rid of strip_pysb somehow. I'd keep that for a separate PR.

@dweindl dweindl mentioned this pull request Nov 1, 2025
@dweindl dweindl added this pull request to the merge queue Nov 1, 2025
Distinguish between `id: str` and `symbol: sp.Symbol`.
The old behavior (`def get_id(self) -> sp.Symbol`) was confusing.

Fix inconsistent type for `AlgebraicEquation` symbol/id.

Closes AMICI-dev#2940.
@dweindl dweindl removed this pull request from the merge queue due to a manual request Nov 1, 2025
@dweindl dweindl enabled auto-merge November 1, 2025 20:46
@dweindl dweindl added this pull request to the merge queue Nov 1, 2025
Merged via the queue into AMICI-dev:main with commit 51996d2 Nov 1, 2025
24 of 25 checks passed
@dweindl dweindl deleted the gh-2940-id-vs-sym branch November 2, 2025 09:27
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.

Add ModelQuantity.sym, make ModelQuantity.get_id return str

2 participants