Skip to content

Fix uncompilable code for sigmas referencing assignment rules - #3388

Merged
FFroehlich merged 5 commits into
mainfrom
claude/issue-3384-b02f04
Oct 15, 2026
Merged

FFroehlich merged 5 commits into
mainfrom
claude/issue-3384-b02f04

Conversation

@FFroehlich

@FFroehlich FFroehlich commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Fixes #3377.
Fixes #3384.
Fixes #3389.
Supersedes #3379.

Problem

Noise formulas (sigmay/sigmaz) that reference expressions (w), e.g., targets of SBML assignment rules, passed import but produced uncompilable code:

dsigmaydp.cpp:12:28: error: 'readout_' was not declared in this scope

The generated sigmay(t, p, k, y) / sigmaz(t, p, k) functions (and JAX _sigmay(y, p, np)) do not receive w. DEModel only checked sigmas for states, so expressions went straight to code generation. As a consequence, dsigmaydp was also wrong, since it lacked the parameter dependence of the expression. This also affected expressions that depend only on parameters (#3377, e.g., sigma = s with s := 0.1 * p) and event noise formulas.

Related cases that were accepted at import but did not compile (#3389): noise formulas with piecewise functions (no Heaviside argument h), splines (no spline values), and state-dependent event noise formulas (states were replaced by the event output z, but sigmaz has no z argument).

Changes

In DEModel._compute_equation, post-processing of sigmay/sigmaz:

The fix lives in DEModel, so it covers the SBML and PySB importers and both the C++ and JAX backends.

Notes for reviewers

  • The error message for unsupported sigmas changed (the old wording, "State-dependent observables are not supported", was misleading). Offending symbols are listed sorted by name, so the message is deterministic. For SBML import, it still reaches users as the __cause__ of the generic SBMLException raised in sbml2amici; I left that wrapping unchanged.
  • The expansion substitutes only the w symbols that actually occur, until none remain, instead of using smart_subs_dict over all of w. That avoids O(n_w) or O(n_w²) substitution work for large models.
  • test_sigma_depending_on_assignment_rules runs with rtol=atol=1e-12. The sigmas there are small and proportional to the observable, so at default tolerances the integration error (~4e-7 relative in the state) is amplified enough to make the finite-difference check miss by ~1e-4. At tight tolerances, FSA, ASA, and finite differences agree to ~1e-9.
  • CI: PEtab Testsuite (3.14) failed on earlier commits of this PR, as it did on main, because libpetab-python#545 changed how numeric overrides are serialized (test_flatten_timepoint_specific_output_overrides). This was fixed on main by Fix flattened observable IDs in PEtab v2 test #3394, which is now merged into this branch.

Testing

New tests in python/tests/test_sbml_import.py:

Locally, the sigma-related tests in test_sbml_import.py pass with GITHUB_ACTIONS=true. The local AMICI build predates the latest C++ changes on main, which do not affect these tests; the full suite runs on CI.

🤖 Generated with Claude Code

FFroehlich and others added 2 commits October 14, 2026 21:13
Noise formulas (sigma{y,z}) referencing expressions, e.g., targets of SBML
assignment rules, passed import, but resulted in uncompilable model code,
since the generated sigma{y,z} functions do not receive `w`. Also, the
derivatives of these expressions were missing from dsigmaydp.

Expressions in sigma{y,z} are now replaced by their definitions before
states are replaced by the respective observable. Sigmas that still depend
on quantities that are not available for their evaluation (states that
cannot be expressed in terms of the respective observable, conservation
law totals, events, splines) are rejected at import with an informative
error instead of failing during compilation.

Fixes #3384.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@FFroehlich
FFroehlich marked this pull request as ready for review October 14, 2026 20:17
@FFroehlich
FFroehlich requested a review from a team as a code owner October 14, 2026 20:17
Copilot AI balanced review requested due to automatic review settings October 14, 2026 20:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings
What changed in this PR

Fixes SBML-imported models producing uncompilable code when sigmay/sigmaz reference assignment-rule expressions (w) by expanding those expressions during equation computation and rejecting unsupported dependencies earlier.

Changes:

  • Expand w (assignment-rule/expression) symbols inside sigmay/sigmaz and related observable/event-observable equations during _compute_equation.
  • Improve validation and error reporting for sigma dependencies that cannot be evaluated (states not eliminable to the respective observable, plus tcl/h/spl).
  • Add a regression test covering sigma formulas depending on assignment rules (including parameter-only rules) and gradient checks.
File Description
python/​sdist/​amici/​_symbolic/​de_model.py Substitutes w symbols in sigmas/observables and reports unavailable symbols with a clearer import-time error.
python/​tests/​test_sbml_import.py Adds regression test for assignment-rule-dependent sigmas, validates rejection and correct sigma/gradient behavior.
changelog.d/​+sigma-expressions.fix.md Documents the fix and the new import-time rejection behavior for unsupported sigma dependencies.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/tests/test_sbml_import.py Resolved
Comment thread python/tests/test_sbml_import.py Resolved
@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.08%. Comparing base (a11074a) to head (ae8ccf7).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3388      +/-   ##
==========================================
+ Coverage   80.06%   80.08%   +0.02%     
==========================================
  Files         320      320              
  Lines       22680    22691      +11     
  Branches     1531     1531              
==========================================
+ Hits        18158    18173      +15     
+ Misses       4514     4510       -4     
  Partials        8        8              
Flag Coverage Δ
cpp 74.20% <100.00%> (+0.03%) ⬆️
cpp_python 35.10% <68.18%> (+0.02%) ⬆️
petab 48.48% <68.18%> (+0.02%) ⬆️
petab_sciml 16.91% <63.63%> (+0.03%) ⬆️
petab_sciml_benchmarks 15.49% <63.63%> (+0.04%) ⬆️
python 72.35% <100.00%> (+0.03%) ⬆️
sbmlsuite 51.02% <ø> (ø)

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

Files with missing lines Coverage Δ
python/sdist/amici/_symbolic/de_model.py 93.82% <100.00%> (+0.30%) ⬆️

... 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.

On CI, conservation laws for non-constant species are enabled
(`GITHUB_ACTIONS`), so `A` is replaced by `tcl_A - B` and the rejected
sigma depends on the conservation law total instead of `A`. The test now
checks both cases explicitly, checks that the accepted sigmas are
resolved with conservation laws, and compiles the model without them.

Also, report unavailable symbols in sorted order, so the error message
does not depend on set ordering.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@dweindl dweindl left a comment

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.

Looks good. Also closes #3377. Addresses points 1 & 2 of #3389 -- also add the last point? Either is fine. Supersedes #3379.

FFroehlich and others added 2 commits October 15, 2026 14:22
sigmaz is evaluated without the event observables, so replacing states
in event noise formulas by the event output generated uncompilable code.
States are now only replaced in sigmay. State-dependent event noise
formulas and event noise formulas that refer to the event observable
itself are rejected at import (#3389, point 3).

Adds `test_noise_formula_with_expression`, adapted from #3379, which
checks sigmay, ssigmay and sigmaz for time-resolved and event noise
formulas that refer to expressions (#3377), and
`test_unsupported_sigmas_rejected` for the cases of #3389 (piecewise
functions, splines, state- and event-observable-dependent event noise
formulas).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@FFroehlich
FFroehlich enabled auto-merge October 15, 2026 14:56
@FFroehlich
FFroehlich added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit 48812fe Oct 15, 2026
38 checks passed
@FFroehlich
FFroehlich deleted the claude/issue-3384-b02f04 branch October 16, 2026 08:58
dweindl added a commit to dweindl/AMICI that referenced this pull request Oct 19, 2026
Event assignments that reference expressions (`w`), e.g., targets of
SBML assignment rules, resulted in uncompilable `deltax` code, as
`deltax` is evaluated without access to `w`. Replace expressions by
their definitions before substituting `x` by `x_old`, reusing the
substitution for noise formulas from AMICI-dev#3388. This also fixes the JAX
backend's `_delta_x`.

Fixes AMICI-dev#3412.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dweindl added a commit to dweindl/AMICI that referenced this pull request Oct 19, 2026
Event assignments that reference expressions (`w`), e.g., targets of
SBML assignment rules, resulted in uncompilable `deltax` code, as
`deltax` is evaluated without access to `w`. Replace expressions by
their definitions before substituting `x` by `x_old`, reusing the
substitution for noise formulas from AMICI-dev#3388. This also fixes the JAX
backend's `_delta_x`.

Fixes AMICI-dev#3412.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dweindl dweindl added the code generation Related to model code generation label Oct 19, 2026
dweindl added a commit to dweindl/AMICI that referenced this pull request Oct 19, 2026
…dev#3416)

Fixes AMICI-dev#3412.

Event assignments that reference expressions (`w`), e.g., targets of
SBML assignment rules (`y := 3 * x`, `E1: at time > 1: v = v + y`),
produced uncompilable code (`deltax.cpp: 'y_' was not declared in this
scope`), since `deltax` is evaluated without access to `w`.

`Event.get_state_update` now takes a function that replaces expressions
by their (recursively expanded) definitions, which is applied before
substituting `x` by `x_old`. This reuses the substitution AMICI-dev#3388 added
for noise formulas, moved to `DEModel._get_expression_substituter`.
Since the fix is in `DEModel`, it also applies to the JAX backend's
`_delta_x` for expressions that only depend on states, parameters and
conservation law totals. JAX's `_delta_x` doesn't get `t` and the
Heaviside variables, so event assignments that depend on `time` or
`piecewise`, directly or via expressions, still fail there, as on `main`
(AMICI-dev#3423).

Event assignments using expressions that contain splines still don't
compile (`deltax.cpp: 'spl_0_' was not declared in this scope`), as on
`main`, since only `w` and `dwdx` get the spline values (AMICI-dev#3424).

The new test `test_event_assignment_with_expressions` uses a chain of
assignment rules depending on a state and a parameter. It checks the
simulated values against the analytical solution, and forward and
adjoint sensitivities against finite differences. On `main`, model
compilation fails.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code generation Related to model code generation

Projects

None yet

3 participants