Add no-ode models#43
Merged
Merged
Conversation
A model with no reactions, ODEs or rate rules has no state variables of its own, but Chaste's ODE system integrates a vector of state variables. Synthesise a single placeholder state variable with dy/dt = 0 so the generated ODE system always has something for the solver to integrate; the model's real outputs (constants, time-dependent assignment rules, event-driven changes) are recomputed each step as before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #43 +/- ##
===========================================
+ Coverage 92.72% 92.96% +0.24%
===========================================
Files 54 54
Lines 6361 6399 +38
===========================================
+ Hits 5898 5949 +51
+ Misses 463 450 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ases When a real SBML id is a C++ keyword or reserved Chaste name (e.g. a parameter `time` or `true`), it is escaped for use as a C++ identifier (`time_`). Previously the escaped identifier was also used as the name reported to Chaste, so the variable could not be looked up by its real SBML id. Record the original id in NameManager and register it as the variable's name, keeping the escaped identifier only in the emitted code. Also re-triage the 253 no-ODE test-suite cases now that no-ODE models are supported: 161 pass, delays/random-event stay unsupported, and remaining failures (event ordering, n-ary relational, codegen gaps, missing MathML) are marked fail_known with notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SBML MathML relationals are n-ary: lt(a, b, c) means a < b < c (a<b and b<c), and eq(a, b, c) means all equal. formulaToL3String renders these as a chained infix expression (a < b < c), which C++ mis-evaluates left to right as (a < b) < c. Rewrite n-ary (>2 operand) relational AST nodes to function-call form before stringifying, so they dispatch to the variadic sm::lt/gt/leq/geq/eq helpers with the correct semantics. Binary relationals stay as infix operators, so existing generated code is unchanged. Marks test-suite cases 01212, 01216, 01494, 01781 as passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the string-based infix-power rewrite (convert_infix_operator_to_ function_syntax / convert_infix_power_to_function_syntax) with an AST pass that converts power nodes to pow() before stringifying. It handles both power node types -- AST_POWER (L3 `^`/`pow`) and AST_FUNCTION_POWER (from MathML `<power>`, used by every reference model) -- and, unlike the string rewrite, converts nested powers correctly (`a^b^c` no longer leaves an inner `^`, which was invalid C++). Also tidy formula_to_string: - hoist the constant/function lookup tables to module scope (were rebuilt on every call); - drop the misleading single-element `unsupported_functions` loop; - normalise a deepCopy so the caller's AST (and the libsbml model) is no longer mutated as a side effect. Regenerate the affected reference models: the only semantic change is pow((x), y) -> pow(x, y); the large line counts are clang-format re-aligning trailing comments on the now-shorter lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move the fragile string/regex handling of special symbols to the AST, before stringifying: - Symbols avogadro, pi, exponentiale and time are renamed to placeholders (replace_constant_symbols), so each stays distinct from a same-named parameter -- fixing a `<pi/>` constant being confused with a parameter named `pi` (case 01819) and removing the hazard of a variable named `t`/`s` being treated as the time symbol. - rateOf is resolved on the AST (resolve_rate_of), robust to the argument's spelling and to nested parentheses that broke the old regex. - _CONSTANTS now maps the capitalised INF/NaN that formulaToL3String actually emits (the old map only had lowercase), and the int->double regex no longer mangles positive exponents (1e+5). Cases 00950/00951/01811 remain failing but are re-noted "non-finite expected results (INF/NaN)": their reference output is literally INF/NaN, which the finite-tolerance test comparison cannot validate -- a test-harness limitation, not a codegen bug. 01819 now passes. Behaviour is unchanged for models without these symbols (the reference generation tests are byte-identical), so no reference models are regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add target_precompile_headers for the heavy header every generated case model shares (AbstractSbmlOdeSystem.hpp) on the project library, and the shared CVODE/test-helper headers on each case test target. Cuts a single-case incremental rebuild's compile time noticeably. Serialization export wrappers and FakePetscSetup are left out (order-sensitive / per-TU). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Degenerate SBML that omits a MathML element used to crash the generator (AttributeError on a null math node). Handle each per SBML semantics, so the model still simulates: - an event with no trigger, or a trigger with no MathML, can never fire, so the event is skipped; - an event delay with no MathML is treated as no delay (zero); - a function definition with no body is skipped (a valid model never calls it). Marks test-suite cases 01238, 01239, 01241, 01271 as passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A species with no reactions, rules or ODE but modified by an event was classified as a derived quantity. Derived quantities are recomputed from the current member each step, so CalculateDerivedQuantitiesAndParameters reported the post-event value at every earlier time point. Model such a species as a (variable) parameter instead -- time-resolved per step, event assignments via the deferred parameter mechanism -- mirroring the boundary-species handling. Fixes cases 00930, 00931, and corrects reference model Chen2004 (BUB2/LTE1/MAD2 move to parameters; its OdeSystem test expectations are updated). The remaining event-ordering cases are marked unsupported (priority ordering, useValuesFromTriggerTime, persistence, competing simultaneous events). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Flux-balance-constraint models cannot be translated to an ODE system. Detect the SBML fbc package and raise NotImplementedError rather than silently generating a model that ignores the constraints. Covers 34/35 flux-balance test-suite cases (no supported case uses fbc). The remaining unsupported categories (event execution semantics, random event execution) cannot be distinguished from supported models by an SBML feature, so are left generating. Also pass -DChaste_UPDATE_PROVENANCE=OFF in infra/configure.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Map SBML 'implies' to a new sm::implies helper (implies(a, b) == (not a) or b), and add single-argument overloads for sm::min/sm::max (the min/max of one value is that value). Both were unmapped and failed to compile. Adds C++ SbmlMath tests for each. Marks cases 01274/01275/01276/01279/01280/01281/01497 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop the parallel _variable_types dict: a variable's VarType is implied by which record collection it belongs to, so derive it (_variable_type_map) instead of maintaining a duplicate updated in five places. - Replace the DerivedQuantity is_conversion/is_reaction boolean pair with a single DerivedQuantityKind enum (NORMAL/REACTION/CONVERSION) -- mutually exclusive by construction, template-friendly, self-documenting. No behaviour change (reference generation output is byte-identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rcsech - sm::xor_ used bitwise ^ directly on its operands, invalid for the double (numeric-boolean) arguments the generated code passes; cast each to bool first. - Add a 2-argument sm::piecewise overload (a piece with no otherwise clause): the value when the condition holds, NaN otherwise -- also the recursion base case for an even argument count. - Map the arcsec/arcsech MathML functions (missing, unlike the other arc* inverses). Adds C++ SbmlMath tests. Marks 01116 and 01486 passing; 01490/01491 compile now but hit a CVODE convergence failure on their degenerate no-ODE models (re-noted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closed
An SBML id spelled like a C++ macro (e.g. a parameter named NAN or INFINITY) cannot be emitted verbatim: `double NAN;` is rewritten by the preprocessor into the macro's expansion (which contains a string literal). Treat these macros like keywords/reserved names in NameManager, so such an id is escaped for the emitted code (NAN -> NAN_) while still reported to Chaste under its original id. Marks 01812 passing; 01813 stays fail_known -- its reference output for a <notanumber/> initial assignment is genuinely NaN, which the finite-tolerance test cannot validate (non-finite expected results). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
libsbml's renameSIdRefs updates variable references but not function-call names (a call f(x) is an AST_FUNCTION node, not an AST_NAME), so renaming a function definition to escape a keyword/reserved/macro clash left its callers invoking the old, now-invalid name. Walk the model's math and rewrite those calls explicitly. Fixes case 01312 (nested function definitions with a function named `this`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reference results and models can be non-finite in two ways, both mishandled: - Expected-results columns may be INF/-INF/NaN (a division by zero, 0/0). These were emitted verbatim into the C++ initialiser list (not valid tokens), and a delta/tolerance check cannot validate them anyway (|INF-INF| is NaN). Emit the <cmath> INFINITY/NAN macros, and compare non-finite expected values by kind and sign instead of by tolerance. - The MathML constants <infinity/> and <notanumber/> are stored by libsbml as ordinary real nodes that render as "INF"/"NaN" -- textually identical to a reference to a parameter so named -- so after stringification they collided with such a parameter and resolved to its value. Detect them by their non-finite value on the AST and rename to placeholders, exactly as pi/avogadro are, so the constant and a same-named parameter stay distinct. Fixes cases 00950, 00951, 01811, 01813. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Initialise() reads each state variable's member to set the solver's initial condition via SetDefaultInitialCondition. Real state variables are assigned their value first (by an INITIAL_VALUE equation), but the synthesised no-ODE placeholder had only a derivative equation -- its member was never assigned, so its initial condition was read from uninitialised memory. When that memory happened to hold a NaN, CVODE integrated from a NaN state and failed with CV_CONV_FAILURE (undefined behaviour that only surfaced on the no-ODE cases that integrate over time rather than solving for a steady state). Add an INITIAL_VALUE equation so the placeholder member is deterministically zeroed before it is used. Fixes cases 01490, 01491. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Collapse all passing-case notes to "ok"; once a case is green the specific
reason it passes is not worth tracking.
- Regroup the unsupported notes so each names one unsupported SBML feature and
cases are grouped consistently:
* merge "delay function" into "delay" -- the event <delay> element and the
delay() csymbol are unsupported for the same reason (no delayed/historical
values) and the split was already inconsistent;
* consolidate the deterministic event cases under "event execution
semantics" (the previous sub-notes misfiled persistence and t0-firing
cases as priority-ordering), keeping the stochastic "random event
execution" cases separate.
- Fix case 01197: it is a trivial 7-dimension-compartment model that passes,
not a flux-balance case -> pass/ok.
- Document in src/cases/README.md (matching test/cases) where case support is
tracked.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-split the semantic-test matrix so each of the 5 batches runs exactly 269 passing cases (was ~230 and unbalanced after retriage). The ranges are contiguous case-id spans covering the whole suite: 1-276, 277-678, 679-1030, 1031-1364, 1365-1821. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements Issue #42 by adding support for SBML “no-ODE” models via a synthetic placeholder state variable with dy/dt = 0, enabling a wider portion of the SBML Test Suite to run. It also hardens SBML→C++ code generation around naming collisions and SBML math edge-cases (constants, n-ary relationals, power, and rateOf).
Changes:
- Synthesize a zero-derivative placeholder state variable when a model has no ODEs/rate rules, rather than rejecting the model.
- Preserve original SBML ids for reporting/output even when C++ identifiers must be escaped (keywords/macros), and extend reserved-name handling to common preprocessor macros.
- Improve SBML math translation and validation: AST-based rewrites for power, n-ary relational semantics, constants (pi/e/inf/nan/time/avogadro),
rateOfhandling, plus matching test-suite harness improvements for non-finite expected values.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| chaste_sbml/tests/test_names.py | Adds coverage for mapping renamed C++ ids back to original SBML ids and macro renaming. |
| chaste_sbml/tests/test_model_builder.py | Adds tests for placeholder state var (no-ODE models) and additional event/package/function-definition edge cases. |
| chaste_sbml/tests/test_expressions.py | Expands tests for power rewriting, n-ary relationals, math constants, implies, and rateOf semantics. |
| chaste_sbml/templates/ode/ode.hpp | Switches derived-quantity member declarations to use DerivedQuantityKind. |
| chaste_sbml/templates/ode/ode.cpp | Reports state/parameter/derived-quantity names using SBML “reported” names (original ids) instead of C++ identifiers. |
| chaste_sbml/templates/cases/semantic.hpp | Improves semantic test comparisons to correctly handle expected NaN/Inf values. |
| chaste_sbml/SbmlRefModels/test/TestSbmlMath.hpp | Adds unit tests for implies, xor numeric-boolean casting, min/max single-arg, and piecewise without otherwise. |
| chaste_sbml/SbmlRefModels/test/reference/TestChen2004SbmlOdeSystem.hpp | Updates expected derived-quantity list/count after generation changes. |
| chaste_sbml/SbmlRefModels/test/data/sbml_test_suite_status.csv | Marks previously “no odes” cases as passing and updates unsupported reasons for some cases. |
| chaste_sbml/SbmlRefModels/test/CMakeLists.txt | Adds precompiled headers for generated case tests to speed rebuilds. |
| chaste_sbml/SbmlRefModels/src/SbmlMath.hpp | Adds implies, fixes xor_ for double operands, adds min/max single-arg overloads, and piecewise base-case for no-otherwise. |
| chaste_sbml/SbmlRefModels/src/reference/TysonNovak2001/TysonNovak2001SbmlOdeSystem.cpp | Regenerated output reflecting power/formatting changes. |
| chaste_sbml/SbmlRefModels/src/reference/Goldbeter1991/Goldbeter1991SbmlOdeSystem.cpp | Regenerated output reflecting power/formatting changes. |
| chaste_sbml/SbmlRefModels/src/reference/Gardner1998/Gardner1998SbmlOdeSystem.cpp | Regenerated output reflecting power/formatting changes. |
| chaste_sbml/SbmlRefModels/src/reference/Chen2004/Chen2004SbmlOdeSystem.hpp | Moves some outputs from derived quantities to parameters to reflect event-modified “constants”. |
| chaste_sbml/SbmlRefModels/src/reference/Chen2004/Chen2004SbmlOdeSystem.cpp | Regenerated output reflecting parameter/derived-quantity changes and event parameter adjustment machinery. |
| chaste_sbml/SbmlRefModels/src/reference/Chen2000/Chen2000SbmlOdeSystem.cpp | Regenerated output reflecting power/formatting changes. |
| chaste_sbml/SbmlRefModels/src/cases/README.md | Updates documentation of what’s covered/supported for case types and where status is tracked. |
| chaste_sbml/SbmlRefModels/infra/configure.sh | Adjusts configure behavior and disables Chaste provenance updates for CI/configure runs. |
| chaste_sbml/SbmlRefModels/generate_cases.py | Ensures reference CSV non-finite tokens compile by mapping INF/NaN to C++ macros. |
| chaste_sbml/SbmlRefModels/CMakeLists.txt | Adds precompiled header for shared heavy include to speed rebuilds. |
| chaste_sbml/_rendering.py | Exposes DerivedQuantityKind to Jinja templates. |
| chaste_sbml/_records.py | Adds name (reported SBML id) fields and replaces conversion/reaction booleans with DerivedQuantityKind. |
| chaste_sbml/_names.py | Adds reserved macro handling and sbml-name back-mapping; fixes function-call renaming when function ids are escaped. |
| chaste_sbml/_model_builder.py | Implements placeholder state var for no-ODE models, adds package/delay/trigger/body guards, switches to kind-based derived quantities, and uses reported SBML names. |
| chaste_sbml/_expressions.py | Refactors translation to AST-based rewrites for constants, rateOf, n-ary relationals, and power; expands SBML function mapping. |
| chaste_sbml/_config.py | Adds PLACEHOLDER_STATE_ID and DerivedQuantityKind. |
| .github/workflows/sbml_test_suite.yml | Updates matrix partitioning to reflect increased number of passing test cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Supports #42