Replace preprocessor macros with mangled local declarations for model equations - #3239
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3239 +/- ##
==========================================
- Coverage 78.77% 77.91% -0.87%
==========================================
Files 318 318
Lines 21247 22183 +936
Branches 1491 1490 -1
==========================================
+ Hits 16738 17284 +546
- Misses 4501 4891 +390
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dweindl
commented
Sep 4, 2026
| @@ -413,6 +413,9 @@ def test_pysb_event(tempdir): | |||
| model = pysb.Model("pysb_event_test") | |||
| a = pysb.Monomer("A") | |||
| pysb.Initial(a(), pysb.Parameter("a0")) | |||
| # "k" is reserved (it's one of AMICI's fixed array-parameter names); | |||
| # unlike SBML import, PySB import has no automatic rename-and-restore | |||
Member
Author
There was a problem hiding this comment.
to be addressed separately. the diff is big enough already.
-> #3243
dweindl
marked this pull request as ready for review
September 4, 2026 13:33
FFroehlich
approved these changes
Sep 4, 2026
… equations Generated model code used unscoped `#define` macros to make equations readable (e.g. `#define STAT x[0]`). This breaks when a model entity's id collides with a C++ keyword, a stdlib macro (`NULL`, `EOF`, ...), or one of amici's own fixed argument names (`x`, `p`, `k`, `t`, ...) -- reserved-word handling only covered 9 hardcoded names and was inconsistent between the SBML and PySB import paths (AMICI-dev#2226). Fix: generate function-scoped local declarations instead of macros -- `const realtype` copies for values read from another function's array (dependencies), and non-`const` references (`realtype &name = array[i];`) bound to a function's own outputs, so their left-hand side stays as readable as the old macros were, with no separate store step. Every identifier is passed through a single mangling+dedup choke point (`AmiciCxxCodePrinter.mangle_identifier`) that guarantees no collision with a keyword/macro and no name reused for two different purposes in the same model. Local scoping also eliminates the unscoped cross-file macro leakage that was the original bug report. `t` and amici's own fixed array-parameter names (`x`, `p`, `k`, `h`, `w`, `y`) still need a pre-codegen rename. `t` because it's also a real merged sympy symbol (SBML's time csymbol); the array-parameter names because the JAX backend has no mangling of its own -- its generated code destructures each array parameter into per-entry locals by reusing the parameter's own name, so an entity actually named e.g. `x` would silently shadow it in that function. Each renamed entity's original id is preserved wherever reported outward (state/parameter ids, PEtab mapping), fixing the id-mismatch bug this previously caused for `t` (AMICI-dev#2461). Also fixes AMICI-dev#3237: amici's own generated names (CSE temporaries, PySB conservation-law totals) no longer contain a reserved double underscore, and a latent bug in the CSE-extraction path for unnamed-array functions (producing invalid C++ like `x[1]_`) found while updating tests for this. Doesn't cover collisions with amici's *other* internally-derived names (e.g. a species called `flux_r1` colliding with a reaction's own flux symbol) -- tracked separately as AMICI-dev#3240. Updates existing tests whose expectations assumed the old macro-based output, drops now-dead reserved-name workarounds, and adds unit and integration test coverage for the new mangling behavior.
dweindl
force-pushed
the
fix-reserved-symbols-2226
branch
from
September 4, 2026 14:42
e161ae2 to
9af1a00
Compare
is_used() re-scanned the whole printed function body per candidate symbol (O(n*m)); now tokenizes once. Also raises install_model reference times: real local declarations cost more to compile than macros did (mostly register allocation), an expected linear trade-off. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dweindl
force-pushed
the
fix-reserved-symbols-2226
branch
from
September 4, 2026 20:51
170f018 to
979479f
Compare
dweindl
commented
Sep 5, 2026
Comment on lines
-5
to
+8
| install_model: 60 | ||
| install_model: 180 | ||
| install_model_O0: 40 | ||
| install_model_O1: 45 | ||
| install_model_O2: 60 | ||
| install_model_O1: 75 | ||
| install_model_O2: 110 |
Member
Author
There was a problem hiding this comment.
@FFroehlich : The additional locals make life more difficult at least for gcc. Not sure about the impact on clang yet (#3244). Either way, I strongly prefer merging this as is. Better slower compilation than silent incorrectness as demonstrated in #3240 (comment).
Member
Author
There was a problem hiding this comment.
Install test model step: gcc vs. clang, before/after PR #3239
5 workflow_dispatch runs each, on main (baseline) vs. fix-reserved-symbols-2226 (this PR), model: CS_Signalling_ERBB_RAS_AKT.
| Task | Compiler | Baseline mean (min–max) | PR mean (min–max) | Δ mean |
|---|---|---|---|---|
install_model (-O3) |
gcc | 34.0s (25.0–41.8s) | 112.3s (87.1–122.2s) | +230% |
install_model (-O3) |
clang | 37.9s (36.0–39.9s) | 39.7s (31.4–44.9s) | +5% |
install_model_O0 |
gcc | 15.4s (11.4–18.6s) | 18.5s (13.1–21.4s) | +20% |
install_model_O0 |
clang | 23.3s (21.3–25.1s) | 21.3s (15.6–24.8s) | −8% |
install_model_O1 |
gcc | 21.0s (15.5–25.7s) | 48.5s (36.7–52.7s) | +131% |
install_model_O1 |
clang | 39.0s (37.2–41.0s) | 35.8s (28.2–40.7s) | −8% |
install_model_O2 |
gcc | 29.1s (21.5–35.4s) | 96.1s (76.9–102.7s) | +230% |
install_model_O2 |
clang | 37.8s (36.1–39.2s) | 39.7s (31.2–45.1s) | +5% |
Summary: the compile-time regression from replacing #define macros with real function-scoped local declarations is gcc-specific. Across all four gcc optimization levels tested, -O1/-O2/-O3 regress 130–230%, while -O0 (minimal register allocation) is only mildly affected (+20%). Clang shows no regression at any level (deltas within run-to-run noise, ±5–8%). This matches profiling with -ftime-report: the added cost is dominated by GCC's register allocator (LRA / integrated RA) handling many newly-real local variables that used to be invisible preprocessor macros — LLVM's allocator does not appear to have the same sensitivity to this pattern.
🤖 Generated with Claude Code
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
2 participants
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.
Generated model code used unscoped
#definemacros to make equationsreadable (e.g.
#define STAT x[0]). This breaks when a model entity's idcollides with a C++ keyword, a stdlib macro (
NULL,EOF, ...), or one ofamici's own fixed argument names (
x,p,k,t, ...) -- reserved-wordhandling only covered 9 hardcoded names and was inconsistent between the
SBML and PySB import paths (#2226).
Fix: generate function-scoped local declarations instead of macros --
const realtypecopies for values read from another function's array(dependencies), and non-
constreferences (realtype &name = array[i];)bound to a function's own outputs, so their left-hand side stays as
readable as the old macros were, with no separate store step. Every
identifier is passed through a single mangling+dedup choke point
(
AmiciCxxCodePrinter.mangle_identifier) that guarantees no collisionwith a keyword/macro and no name reused for two different purposes in the
same model. Local scoping also eliminates the unscoped cross-file macro
leakage that was the original bug report.
tand amici's own fixed array-parameter names (x,p,k,h,w,y) still need a pre-codegen rename.tbecause it's also a real mergedsympy symbol (SBML's time csymbol); the array-parameter names because the
JAX backend has no mangling of its own -- its generated code destructures
each array parameter into per-entry locals by reusing the parameter's own
name, so an entity actually named e.g.
xwould silently shadow it inthat function. Each renamed entity's original id is preserved wherever
reported outward (state/parameter ids, PEtab mapping), fixing the
id-mismatch bug this previously caused for
t(#2461).
Also fixes #3237: amici's own
generated names (CSE temporaries, PySB conservation-law totals) no longer
contain a reserved double underscore, and a latent bug in the
CSE-extraction path for unnamed-array functions (producing invalid C++
like
x[1]_) found while updating tests for this.Doesn't cover collisions with amici's other internally-derived names
(e.g. a species called
flux_r1colliding with a reaction's own fluxsymbol) -- tracked separately as
#3240.
Updates existing tests whose expectations assumed the old macro-based
output, drops now-dead reserved-name workarounds, and adds unit and
integration test coverage for the new mangling behavior.
🤖 Generated with Claude Code