Fix SBML import constant-folding parameters out of initial conditions - #3216
Open
FFroehlich wants to merge 3 commits into
Open
Fix SBML import constant-folding parameters out of initial conditions#3216FFroehlich wants to merge 3 commits into
FFroehlich wants to merge 3 commits into
Conversation
…#3214) A parameter that both carries an initialAssignment with a numeric right-hand side and defines a species' initial value was constant-folded out of x0 by `_make_initial`: the parameter's initial-assignment value was substituted in, dropping the symbolic dependence. The parameter remained in the free-parameter list, so its initial-condition sensitivity was silently reported as zero (e.g. `init_STAT` in `jakstat_adjoint`). `_make_initial` now leaves symbols that AMICI keeps as free or fixed parameters untouched, since their value has already been captured as the parameter's nominal value in `_process_parameters`. This matches the existing design, where x0 is expected to be expressed in terms of free and fixed parameters (and time). Add a regression test covering the minimal reproducer from the issue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xf5RqvErY7po9N9DNfpKQ
Extend the `_make_initial` guard so a parameter reached only through a chain of parameter initial assignments is not constant-folded either. `par_id_to_ia` in `_process_parameters` calls `_make_initial` before the free/fixed-parameter dicts are populated, so the guard now also recognizes a parameter directly from the SBML model (excluding parameters that become states). A parameter whose numeric value derives from another free parameter is now classified as an expression rather than an independent, constant-folded free parameter, keeping the initial-condition sensitivity. Also assert that the regression test's premise holds (the parameter really carries an initial assignment), guarding against a silent false negative if antimony/libSBML ever folds the compound right-hand side to a plain value, and add a regression test for the chained-parameter variant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xf5RqvErY7po9N9DNfpKQ
Cover the FIXED_PARAMETER branch of the initial-condition guard: a constant parameter that both carries a numeric initialAssignment and defines a species' initial value must keep its symbol in x0, so changing the constant still moves the initial condition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xf5RqvErY7po9N9DNfpKQ
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an SBML-import bug where parameters that have initialAssignment definitions could be substituted into species initial conditions (x0), causing the generated model to lose symbolic dependence on those parameters and therefore report zero initial-condition sensitivities even though the parameters remain in the free-parameter list (issue #3214).
Changes:
- Updated
_make_initial()to skip substituting initial assignments for symbols that represent parameters AMICI keeps symbolic (free/fixed parameters and parameter expressions), preventing constant-folding intox0. - Added two regression tests covering both the direct and chained-parameter initial-assignment cases to ensure
x0retains parameter dependence. - Documented the fix in the v1.0.2 (unreleased) changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/sdist/amici/importers/sbml/init.py | Prevents substituting parameter initialAssignments during initial-condition construction to preserve symbolic parameter dependence in x0. |
| python/tests/test_sbml_import.py | Adds regression tests for direct and transitive initial-assignment parameter folding into x0. |
| CHANGELOG.md | Notes the SBML import fix and its impact on sensitivities, referencing #3214. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3216 +/- ##
==========================================
- Coverage 78.46% 77.84% -0.63%
==========================================
Files 317 317
Lines 20974 20976 +2
Branches 1483 1482 -1
==========================================
- Hits 16458 16328 -130
- Misses 4508 4640 +132
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed a bug in SBML import where parameters with
initialAssignmentelements were being constant-folded out of initial conditions, causing their sensitivities to be silently reported as zero even though they remained in the free-parameter list.Key Changes
Modified
_make_initial()insbml/__init__.py: Added logic to skip substitution of initial assignments for parameters that AMICI keeps as symbolic quantities (free/fixed parameters or expressions). This prevents constant-folding of parameter dependencies into initial conditions.Added regression tests in
test_sbml_import.py:test_initial_assignment_parameter_not_constant_folded(): Tests the case where a parameter with a compound-expression initial assignment (e.g.,X0 = 6 * 1) defines a species' initial conditiontest_chained_initial_assignment_parameter_not_constant_folded(): Tests the transitive case where a parameter's initial assignment references another parameter with its own initial assignmentImplementation Details
The fix recognizes that during parameter classification in
_process_parameters, certain parameters are intentionally kept as symbolic quantities rather than being folded to numeric values. When processing initial conditions, the code now skips substituting initial assignments for such parameters, preserving the symbolic dependence chain. This ensures that sensitivity analysis correctly reports non-zero derivatives with respect to these parameters.The solution handles both direct cases (a parameter with a numeric initial assignment) and transitive cases (chains of parameter initial assignments), addressing issue #3214.
https://claude.ai/code/session_012xf5RqvErY7po9N9DNfpKQ