SBML Test Suite [Phase 5]#33
Merged
Merged
Conversation
Boundary-condition species are not changed by reactions; their value is held constant or set by a rule. Building a reaction ODE for one also collided with its rate rule, raising a spurious "more than one rate rule and/or reaction" error (case 1538). Skip reaction contributions for boundary species and let a rate rule take precedence over any reaction contribution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A reaction's ID denotes its rate (flux), an observable that rules, events and test outputs can read (e.g. case 1344's "A__x = J1"). Register every reaction flux as a derived quantity, ordered after the model-intrinsic quantities and before the amount/concentration conversions so the former keep stable indices. The flux member is already declared and computed by the reaction machinery, so it keeps its VarType.REACTION and is not re-declared (is_reaction flag). Regenerate the reference models accordingly and bump the hardcoded derived- quantity counts (Chen2004 93->187, TysonNovak2001 16->36); the named/index checks are unchanged as the model-intrinsic quantities still come first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
generate_references.py (with an infra/generate_references.sh wrapper) regenerates every reference model from its SBML source, deriving the model name from the file name and inferring the model type from the companion SRN/cell-cycle files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #33 +/- ##
===========================================
- Coverage 89.99% 89.27% -0.73%
===========================================
Files 48 48
Lines 5619 6320 +701
===========================================
+ Hits 5057 5642 +585
- Misses 562 678 +116 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A reaction's local parameter can become unused when its only reference folds away, e.g. rateOf() of a constant local parameter evaluates to 0 (case 1459), leaving the emitted declaration unused and tripping -Werror=unused-variable. Mark local-parameter declarations [[maybe_unused]]. Also record statuses for cases 1345-1459: passes for the reaction-flux and rateOf cases, unsupported for the algebraic-rule (1350, 1386) and fast-reaction (1396, 1397, 1399) cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An event assignment with no MathML (e.g. case 1605's "R =") assigns nothing, leaving its target unchanged. getMath() returns null for these, which crashed _strip_ast_units; skip them instead. Record case 1605 as unsupported (random event execution): it is a stochastic test whose competing same-priority events fire at random, which a deterministic solver cannot reproduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A reaction's local parameter shadows any global symbol of the same name and is constant, so rateOf applied to it is zero. The handler only checked the global variable type, so rateOf(k1) wrongly resolved to a global k1's rate rather than the local constant (case 1460). Pass the equation's local parameters into _formula_to_string and return 0.0 for rateOf of a local parameter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pick up the [[maybe_unused]] annotation on reaction local-parameter declarations. Purely cosmetic; affects only the three models with local parameters (Chen2004, Gardner1998, Goldbeter1991). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
formulaToL3String emits bare plus()/times() for empty n-ary MathML operators (the additive/multiplicative identities 0 and 1, used in case 1531's uncommon- MathML event triggers). These were not in custom_functions, so they compiled to undeclared bare calls. Route them through sm::plus()/sm::times(), whose variadic templates already fold to 0 and 1. Case 1531 now compiles and most events fire, but it remains a known failure: events firing before/around the trigger's zero crossing are missed by CVODE event detection when 56 events share one monotonic trigger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A species-reference stoichiometry driven by a rate rule is a state variable; its initial assignment overrides the speciesReference stoichiometry attribute. The initial-assignment handler only applied to stoichiometries that became parameters (constant stoichiometry), so a variable stoichiometry kept its default attribute value (case 1543: S1_stoich stuck at 1 instead of 1/2, skewing rateOf(S1)). Apply the initial assignment for state-variable stoichiometries too; Initialise's SetDefaultInitialCondition then carries it into the initial conditions. Record passes for cases 1527, 1540, 1542, 1543. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-ran the 123 cases marked partly_supported/event alignment. The event- detection work on this branch (max-combination root function, monotonic clamp, sticky fire, grid-driven harness) fixed 100 of them, now recorded as passing. Three are now unsupported at generation (delays 1349/1385, fast reaction 1398). Twenty still fail and remain partly_supported/event alignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A constant boundary species (no rule, non-time-varying compartment) was emitted as a derived quantity. An event that assigns it then mutated a member that the recorded solution never stored, so its amount conversion (amt__X = X * compartment) was recomputed with the species' final value at every point - the event timing was lost (e.g. cases 375-383). Model such a species as a (variable) parameter instead, so its value is stored per step and event assignments use the deferred parameter mechanism. Add SbmlTestOdeSolution::GetDerivedQuantitySeries, which restores each step's parameters before evaluating a derived quantity, and route derived quantities through it in the test harness so parameter-dependent conversions are time-resolved. Records passes for cases 375, 377, 378, 380, 381, 383. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
For a relational trigger the fire decision used the raw SBML condition (e.g. time >= 5), which turns true an epsilon before the signed event distance (time - 5 - eps) reaches zero. When the crossing coincides with a sample grid point, an uncommitted root-function evaluation at the grid point latched the event satisfied before it could be applied, so the assignment never took effect (e.g. cases 1444-1448: an event reducing a reaction's summed variable stoichiometry at time>=5 never fired, leaving dA/dt too high). Add a has_distance flag and, for relational triggers, drive the clamp and fire logic from (event_dist >= 0) - consistent with CVODE's root threshold - instead of the raw condition. Compound/opaque triggers keep using the raw condition. Regenerate the reference models (Chen2004, TysonNovak2001 have events); their tests still pass. Records passes for cases 1444-1448. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An event that resizes a compartment leaves the amounts of the concentration species in it unchanged (SBML semantics), so their concentration must be rescaled by old_size / new_size. The codegen tracks such species as concentration, so without this their amount (concentration * size) jumped with the compartment (cases 945-948: amt__S1 ~10x too high after C 1->10). When an event assigns a compartment - directly, or indirectly via an assignment rule fed by an event-assigned variable - append compensating concentration rescale assignments for its species. Add _collect_ast_names / _substitute_ast_names helpers to compute the indirect new size. Records passes for cases 945-948. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When several events fire at the same time, SBML executes them in priority order (higher priority first), so the lowest-priority event executes last and its assignment wins any conflict. Priority was a TODO - events applied in declaration order, so the wrong value won (cases 1169, 1267, 1298: two same-trigger events assigning one variable). Read the event priority and, when deferring an assignment, only overwrite one already made this firing if the new event's priority is lower-or-equal (tracked in new mEventAdjusted*Priority arrays). Events without a priority use +inf, reducing to the previous last-writer-wins behaviour. Regenerate the reference models (Chen2004, TysonNovak2001); their (priority-less) event handling is unchanged and their tests pass. Records passes for 1169/1267/1298. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CvodeAdaptor's default tolerances (rtol 1e-4) leave an integration error comparable to the test tolerance (1e-4), which fails in stiff transients - e.g. case 976, where a reaction rate k * S1 / S2 is sensitive to a small S2, gives a ~0.5% error at the first sample. Set rtol 1e-7 / atol 1e-9 in the harness; tighter tolerances only make passing cases more accurate. This also resolves case 382, whose event crossing sat a hair on the wrong side of a sample grid point. Records passes for cases 382 and 976. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-distribute the five batches so each runs an equal number of supported (non-unsupported) cases. The split had drifted to 233/232/231/229/224 as case statuses changed; the new ranges hold exactly 230 supported cases each. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rtol 1e-7 made CVODE resolve a discontinuous rate (e.g. case 28's ceil/factorial) differently from the reference integrator, regressing it. rtol 1e-6 / atol 1e-8 is still tight enough for the stiff transients that motivated tightening (case 976) and keeps the grid-precision case 382, while restoring case 28 and the other ceil/floor/piecewise cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous fix drove an event's fire decision entirely from the signed distance (distance >= 0). For a trigger that is already true at t=0 with initialValue=true (e.g. case 929's t >= 0), the distance is -eps at t=0, so the event was treated as inactive, the satisfied latch cleared, and the event then fired - though SBML says it must not (no false->true transition). Track two separate booleans: active (the raw condition) drives the satisfied/re-arm state and the initial-value handling, while detected (distance >= 0) gates only the fire and the latch. So an event already true at t=0 stays satisfied and never fires (929), while one whose crossing lands on a sample grid point still localizes at the distance zero-crossing rather than latching early (1444-1448). Drop the now-unused has_distance flag. Regenerate the reference models; their tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two fixes for local parameters that share a name with a reaction member (cases 1799-1802): - A local parameter named like its reaction's ID (1799) shadowed the reaction member in the generated block, so the kinetic-law assignment wrote to the local and left the member (the flux) unset. Qualify the assignment with this-> so it targets the member while the RHS still reads the shadowing local. - A named species reference whose stoichiometry is left unset defaults to 1, but it was only initialised when isSetStoichiometry() was true, so it stayed 0 (1800-1802). Use getStoichiometry() (the default) whenever it is a real number. Regenerate the reference models (only the cosmetic this-> qualification changes); their tests pass. Records passes for cases 1799-1802. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The avogadro csymbol and a parameter (or local parameter) named 'avogadro' are distinct AST nodes but render identically, so a rate like avogadro / avogadro (csymbol over parameter) collapsed to a single value (cases 1760, 1762, 1763). Rename avogadro csymbol nodes to a placeholder mapped to sm::AVOGADRO, keeping them distinct from a same-named identifier. Also exclude local-parameter names from the constant replacement so a local parameter named 'avogadro' stays the local rather than becoming sm::AVOGADRO. Records passes for cases 1760, 1762, 1763. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A conversion factor relates a reaction's extent to the change in a species' amount, so it applies to reaction-driven species only. A rate rule gives the species' rate of change directly, so the factor must not be applied - but it was multiplied into every state-variable species, including rate-rule ones (cases 1666-1668: dS1/dt = -0.01 * s1_cf instead of -0.01). Skip the conversion factor when the species is under a rate rule. Records passes for cases 1666-1668. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ned species on resize Two fixes for an event that resizes a compartment while a species and the compartment are both under rate rules (cases 1505, 1506): - The deferred event-assignment values were recorded only at first detection, which under a bounded max step is an integration step past the true crossing. A state-dependent value (e.g. a compartment-resize rescale S * C_old / C_new) was therefore evaluated at the wrong state. Record the values on every ProcessModelEvents call while the event has fired this segment, so CVODE's root-localisation calls leave them evaluated at the committed root. This is driver-independent (no test-harness coupling). - When an event assigns a concentration species its compartment is also resizing, the assigned concentration is taken at the old size, so scale it by old_size / new_size to carry the resulting amount across the resize (1506). Regenerate the reference models. The more accurate event localisation shifts one Chen2004 cell-cycle value (ORI, set by an event) by ~0.01; update its expected value. All ODE, cell-cycle and SRN reference tests pass. Records passes for 1505, 1506. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Case 884 (an event firing exactly at a sample grid point) now passes: recording event assignments at the localized root rather than the integration step endpoint fixed its timing. It was the last partly_supported event-alignment case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-checked every unsupported entry against the codegen's actual behaviour (captured by running it over the whole suite): - 39 cases marked 'unsupported,no odes' actually generate and pass (they have ODEs) - reclassify them as pass. - Fill 48 empty notes and fix wrong reasons (e.g. delay vs algebraic rule, no odes vs delay/algebraic rule, delay vs fast reaction) to match the reason the codegen reports when it skips the case. Flux-balance (no time course) and random-event-execution cases keep their descriptive notes. No fail_known/fail_unknown cases remain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the 10x inflation of the absolute and relative tolerances and the 1e-6 tolerance floor, so generated cases are checked at the SBML Test Suite's own declared tolerances. A 100-case spot check across the pass set still passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These cases were swept from fail_* to pass by an earlier bulk update, but the codegen cannot generate any of them: the SBML version it selects uses an unsupported feature (no odes, delay, or fast reaction), so they never ran. A generation sweep of the whole suite cross-checked against the pass rows found all 35; correct them to unsupported with the codegen's actual reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add infra/check_passing_cases_generated.py and a workflow step that fails a batch when any case marked pass in the status CSV generated no test, catching the misclassification fixed in the previous commit before it can silently stop running. Rebalance the matrix so each batch runs ~230 passing cases now that the pass set has shrunk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A single global solver tolerance left small-amount cases with tight absolute tolerances (e.g. case 66, absolute 1e-9) integrated too loosely, so their error exceeded the suite bound once the artificial 1e-6 test-tolerance floor was removed. Set CVODE to a hundredth of each case's declared absolute and relative tolerances instead: tight-tolerance cases get an accurate solve, while loose-tolerance cases keep a loose solver, which discontinuous rates (e.g. case 28's ceil/factorial) need to avoid diverging from the reference integrator. Verified against all 171 passing cases with absolute tolerance <= 1e-7 and a 130-case spread across the suite; the only failure is the already partly_supported case 1531. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A hundredth of the test tolerances left exponentially growing comp cases (e.g. 1145, 1148, 1159-1161, where a submodel species grows ~20-50 fold per unit time) short of the suite bound: integration error accumulates through the transient faster than the local tolerance suggests. Solve to a ten-thousandth of each case's tolerances instead, deriving both the solver and comparison tolerances from the same tol_absolute/tol_relative. Re-validated against a 264-case union (the 100-spread, all 171 tight-absolute cases, the comp cases, and the discontinuous/stiff/event canaries); the only failure is the already partly_supported case 1531. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
All supported test suite cases are now passing. |
kwabenantim
marked this pull request as ready for review
June 11, 2026 08:15
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
1 participant
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
Supports #21, following #28, #30, #31, and #32 to fix failing tests in batch 5 of sbml_test_suite.yml