Skip to content

Conversation

@FFroehlich
Copy link
Member

@FFroehlich FFroehlich commented Jun 28, 2025

Summary

  • avoid reserved name renaming during SBML import
  • drop reserved name check in ModelQuantity
  • sanitize symbols when printing C++ and JAX code
  • adjust sbml tests for new behavior
  • fix reserved symbol handling during code generation
  • allow BNGL conversion for empty-compartment tests
  • fix JAX SBML test module import
  • revert SBMLSuiteJax import path
  • use semantic-results-jax directory for JAX SBML suite results
  • fix parameter ID names
  • fix parameter ID handling
  • fix JAX sanitization of variable names

Testing

  • pre-commit run --files python/sdist/amici/jax/ode_export.py
  • pytest python/tests/test_sbml_import.py::test_import_same_model_name -vv -s
  • pytest python/tests/test_events.py::test_handling_of_fixed_time_point_event_triggers -q
  • pytest tests/benchmark_models/test_petab_benchmark.py -k Boehm_JProteomeRes2014 -q
  • pytest tests/benchmark_models/test_petab_benchmark_jax.py -k Boehm_JProteomeRes2014 -q
  • pytest tests/sbml/testSBMLSuiteJax.py::test_sbml_testsuite_case_jax[00525] -q

fixes #2461


https://chatgpt.com/codex/tasks/task_b_685d2802e714832b968ced9c529d9411

@codecov
Copy link

codecov bot commented Jun 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.27%. Comparing base (08bf630) to head (4e43dc7).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2853      +/-   ##
==========================================
- Coverage   80.38%   80.27%   -0.11%     
==========================================
  Files         329      329              
  Lines       22410    22413       +3     
  Branches     1514     1514              
==========================================
- Hits        18014    17993      -21     
- Misses       4385     4409      +24     
  Partials       11       11              
Flag Coverage Δ
cpp 75.41% <92.85%> (-0.11%) ⬇️
cpp_python 33.79% <42.85%> (-0.05%) ⬇️
petab 39.21% <85.71%> (-0.04%) ⬇️
python 74.44% <92.85%> (-0.11%) ⬇️
sbmlsuite-jax 33.83% <57.14%> (-0.19%) ⬇️

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

Files with missing lines Coverage Δ
python/sdist/amici/cxxcodeprinter.py 95.68% <100.00%> (+0.19%) ⬆️
python/sdist/amici/de_export.py 97.43% <100.00%> (+<0.01%) ⬆️
python/sdist/amici/de_model_components.py 96.40% <ø> (-0.03%) ⬇️
python/sdist/amici/jax/jaxcodeprinter.py 90.62% <100.00%> (+1.73%) ⬆️
python/sdist/amici/jax/ode_export.py 98.03% <100.00%> (+0.08%) ⬆️
python/sdist/amici/sbml_import.py 93.20% <ø> (-1.00%) ⬇️

... and 1 file with indirect coverage changes

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

@FFroehlich FFroehlich marked this pull request as ready for review June 28, 2025 15:15
@FFroehlich FFroehlich requested a review from a team as a code owner June 28, 2025 15:15
@sonarqubecloud
Copy link


def _print_Symbol(self, expr: sp.Symbol) -> str:
name = super()._print_Symbol(expr)
if name in RESERVED_SYMBOLS and name != "t":
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a model has a parameter t that is not time?

Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

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

I'd be happy to handle reserved symbols only during code generation, but I don't think this will work well in general. The problem is that we don't know anymore whether the given symbol is a user-defined symbol or an amici-internal symbol. There are quite a number of (currently unhandled) names that will cause problems, that we can't handle here (d{}dt, xdot{}, stau{}, any C++ keywords and typenames, any parameter names from amici's model functions, ...).
I think the most and only sustainable solution would be some string-based indexing that is evaluated at compile-time that avoids any collisions (see also #2226).

@FFroehlich FFroehlich marked this pull request as draft July 1, 2025 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle reserved names in PEtab parameter mapping

2 participants