code restructure by kwabenantim · Pull Request #38 · Chaste/chaste-codegen-sbml

kwabenantim · 2026-07-04T03:30:09Z

Supports #35

Clear each parameter's adjustment flag after AdjustParameters applies it, symmetric with the existing state-variable handling. On the CVODE path ProcessModelEvents never clears these flags, so a set parameter flag would otherwise re-apply its (stale) value at the start of every subsequent Solve segment, clobbering any later change to that parameter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

codecov · 2026-07-04T03:59:52Z

Codecov Report

❌ Patch coverage is 82.22892% with 177 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.93%. Comparing base (9868042) to head (80ef04a).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
chaste_sbml/_model_builder.py	76.13%	132 Missing ⚠️
chaste_sbml/_expressions.py	80.60%	32 Missing ⚠️
chaste_sbml/_sbml_loader.py	56.52%	10 Missing ⚠️
...e_sbml/SbmlRefModels/src/AbstractSbmlOdeSystem.cpp	75.00%	1 Missing ⚠️
...bml/SbmlRefModels/src/fortests/SbmlTestHelpers.hpp	80.00%	1 Missing ⚠️
chaste_sbml/_names.py	98.68%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #38      +/-   ##
===========================================
+ Coverage    89.19%   91.93%   +2.74%     
===========================================
  Files           49       55       +6     
  Lines         6346     6364      +18     
===========================================
+ Hits          5660     5851     +191     
+ Misses         686      513     -173

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Includes: add missing <algorithm> (std::fill/std::any_of) to AbstractSbmlOdeSystem.cpp; drop headers left unused after the solver-setup refactor (CVODE/RK4/BackwardEuler solver headers now come via SbmlOdeSolverSetup.hpp), plus unused <iostream>/<limits>/<fstream>, ChasteSerialization, and the proliferative-type headers. C++17: use std::any_of for the event-trigger scan, range-based for loops in ExportCsv, and = default for the abstract classes' destructors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

SbmlMath.cpp was removed (its functions are now inline in SbmlMath.hpp), so test_base_class_files_found can no longer assert it is among the shipped base-class files. Assert AbstractSbmlOdeSystem.cpp instead, keeping the check that a .cpp is included. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Phase A of resolving variable name conflicts: add a detector that fails loudly rather than emit silently-incorrect C++. A new _names module defines the C++ keywords and Chaste base-class member names that generated identifiers must not use, plus find_name_conflicts() which reports duplicates, keywords, reserved names and invalid identifiers. _check_name_conflicts() gathers every identifier the templates emit (parameters, state variables and their derivatives, derived quantities including amount/concentration conversions, stoichiometry variables, reactions and functions) and raises NameConflictError on any conflict; it runs at the end of _process_model. Initial assignments are excluded from the check: their id is the existing variable they assign to, not a new declaration. Verified generation-neutral on the reference models and against the semantic SBML test suite (no false positives). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR introduces explicit C++ identifier conflict detection during SBML→Chaste code generation (supporting issue #35), and refactors the shipped C++ base/reference code to remove now-unneeded hooks and centralize ODE solver defaults.

Changes:

Add a name-conflict detection module (_names.py) and call it during model processing to fail early on C++ identifier collisions/keywords/reserved Chaste names.
Restructure/trim generated and reference C++ ODE system interfaces (remove unused rule/reaction update hooks) and inline simple SBML math helpers into SbmlMath.hpp.
Centralize SBML ODE solver default configuration in a new header and expand CI workflow triggers (plus a README badge).

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
README.md	Adds CI badge for the SBML test suite workflow.
chaste_sbml/tests/test_names.py	Adds unit tests for identifier validity and conflict reporting.
chaste_sbml/tests/test_chaste_sbml_model.py	Adds tests for model-level conflict checking and error raising.
chaste_sbml/tests/test_base_classes.py	Updates expectations for shipped base-class file set.
chaste_sbml/templates/ode/ode.hpp	Removes declarations for hooks no longer present in base ODE interface.
chaste_sbml/templates/ode/ode.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/test/reference/TestTysonNovak2001SbmlCellCycleModel.hpp	Updates one expected numeric reference value.
chaste_sbml/SbmlRefModels/src/SbmlOdeSolverSetup.hpp	Adds shared helper to configure default ODE solver/tolerances/steps.
chaste_sbml/SbmlRefModels/src/SbmlMath.hpp	Inlines previously out-of-line math helpers (and keeps templates).
chaste_sbml/SbmlRefModels/src/SbmlMath.cpp	Removes now-unneeded implementation file (logic moved inline).
chaste_sbml/SbmlRefModels/src/SbmlEventType.hpp	Adds license header block.
chaste_sbml/SbmlRefModels/src/reference/VanLeeuwen2007NonDim/VanLeeuwen2007NonDimSbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/VanLeeuwen2007NonDim/VanLeeuwen2007NonDimSbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/VanLeeuwen2007/VanLeeuwen2007SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/VanLeeuwen2007/VanLeeuwen2007SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/TysonNovak2001/TysonNovak2001SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/TysonNovak2001/TysonNovak2001SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Tan2014/Tan2014SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Tan2014/Tan2014SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Goldbeter1991/Goldbeter1991SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Goldbeter1991/Goldbeter1991SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Gardner1998/Gardner1998SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Gardner1998/Gardner1998SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Chen2004/Chen2004SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Chen2004/Chen2004SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Chen2000/Chen2000SbmlOdeSystem.hpp	Removes declarations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/reference/Chen2000/Chen2000SbmlOdeSystem.cpp	Removes stub implementations for removed ODE hooks.
chaste_sbml/SbmlRefModels/src/fortests/SbmlTestHelpers.hpp	Fixes quantile implementation to match reference-statistics generation.
chaste_sbml/SbmlRefModels/src/fortests/SbmlTestHelpers.cpp	Improves CSV export header writing and reduces flush overhead.
chaste_sbml/SbmlRefModels/src/cases/.gitignore	Ignores generated SBML test-suite case directories.
chaste_sbml/SbmlRefModels/src/AbstractSbmlSrnModel.hpp	Removes redundant `SimulateToCurrentTime` override declaration.
chaste_sbml/SbmlRefModels/src/AbstractSbmlSrnModel.cpp	Uses shared solver setup helper and defaults the destructor.
chaste_sbml/SbmlRefModels/src/AbstractSbmlOdeSystem.hpp	Removes now-unneeded virtual hook declarations and `RunModelRules`.
chaste_sbml/SbmlRefModels/src/AbstractSbmlOdeSystem.cpp	Clears parameter/state event-adjust flags after applying; simplifies stopping-event check.
chaste_sbml/SbmlRefModels/src/AbstractSbmlCellCycleModel.cpp	Uses shared solver setup helper and defaults the destructor.
chaste_sbml/chaste_sbml_model.py	Adds `_check_name_conflicts()` during model processing and raises `NameConflictError` on conflicts.
chaste_sbml/_names.py	Adds keyword/reserved-name/duplicate detection utilities and error type.
.github/workflows/sbml_test_suite.yml	Runs workflow on PRs and develop pushes; updates trigger structure.
.github/workflows/build_and_test.yml	Runs workflow on PRs and develop pushes; updates trigger structure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Phase B of resolving variable name conflicts: keep generator-synthesised C++ identifiers collision-free. _collect_taken_names() gathers the real ids that become C++ names (species, global/local parameters, compartments, reactions, function definitions, stoichiometry refs), and _reserve_synthetic() allocates each synthetic name (state derivatives d_x_dt, amount/concentration conversions amt__x/conc__x) against those plus the reserved Chaste names via the new unique_name() helper. Collision-only: a synthetic keeps its clean name unless a real id genuinely occupies it, in which case the smallest free _N suffix is appended (the real id is left untouched). Reference model output is unchanged. Resolves the existing "check for name clashes with derivative_id's" TODO. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.

_check_name_conflicts() now checks each reaction's local-parameter ids for C++ keywords and invalid identifiers (they are emitted as `double <id> = ...;`), closing a gap where e.g. a local parameter named `int` would fail to compile without being caught. Local parameters are checked in isolation (keywords and validity only), so legal per-reaction-scoped ids that repeat or shadow a global are not flagged. Adds regression tests and sets _equations in the test helper. Also drop black's target-version, which is redundant with requires-python. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Phase C of resolving variable name conflicts: rename real SBML ids that cannot be emitted as C++ verbatim -- those equal to a C++ keyword (e.g. a compartment called `default`) or a Chaste base-class member. Since an SBML SId is already valid C++ identifier syntax, resolve_cpp_name() only escapes such clashes by suffixing `_` and then uniquifies. _resolve_name_conflicts() applies this at the SBML level before processing: setId plus per-element renameSIdRefs rewrites every reference, so equations, events, initial assignments and the templates see only clean ids and need no changes. Only global SId-namespace entities that become C++ identifiers are renamed; safe ids are untouched so conflict-free models (including the reference models) are byte-identical. Verified against the SBML test suite: the semantic cases are unchanged and the syntactic keyword cases (e.g. `default`) now generate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Bring two docstrings in line with the implemented behaviour: _check_name_conflicts now documents that it also checks kinetic-law local parameters (keywords/validity only, as they are per-reaction-scoped), and the _names module docstring describes all three roles it grew -- detection (find_name_conflicts), unique synthetic-name allocation (unique_name) and real-id resolution (resolve_cpp_name) -- rather than claiming to only diagnose conflicts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.

Local kinetic-law parameters were checked only for C++ keywords and validity, so a local parameter named `time` slipped through -- yet `time` is emitted as the simulation-time function parameter, and a local `double time = ...;` would shadow it and silently corrupt any time-dependent reaction expression. Check local parameters against the reserved Chaste names too (they remain exempt from duplicate detection, so ids that repeat across reactions or shadow a global stay legal). Updates the docstring and adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move the SBML MathML/AST -> C++ formula translation out of ChasteSbmlModel into a new _expressions module: the AST helpers (collect/substitute names, replace the avogadro csymbol, strip units), the infix-operator rewriter and formula_to_string are now free functions. formula_to_string takes the model's variable-type map and state variables as arguments rather than reaching into the model, and resolves rateOf inline. ChasteSbmlModel keeps a thin _formula_to_string wrapper so its call sites are unchanged. Behaviour-neutral: reference model output is byte-identical. The infix tests move to test_expressions.py alongside new formula_to_string tests. Trims ChasteSbmlModel by ~350 lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move the SBML-model-level naming lifecycle out of ChasteSbmlModel into a NameManager class in _names: resolving real ids that clash with C++ keywords or reserved names (phase C), reserving collision-free synthetic names (phase B) and collecting the real ids to avoid. ChasteSbmlModel holds a NameManager and delegates (resolve_real_id_conflicts, reset, reserve); the _taken_names attribute is gone. _check_name_conflicts (phase A) stays as model self-validation. Behaviour-neutral: reference model output is byte-identical. The reserve/resolve tests move to test_names.py as NameManager tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move the model-agnostic templating mechanics out of ChasteSbmlModel into a CodeRenderer in _rendering: the Jinja environment and globals, template rendering, and writing the outputs to disk (formatted with clang-format). The model keeps the model-specific parts -- assembling the template context and choosing which templates by model type -- and delegates rendering and writing to the renderer. Behaviour-neutral: reference model output is byte-identical. Removes the now-unused jinja2/pathlib/shutil/subprocess/sys imports from the model. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move reading, comp-flattening and converting the SBML file out of ChasteSbmlModel.__init__ into load_sbml_model in _sbml_loader, which returns a ready-to-process libsbml Model. python-libsbml keeps the owning document alive through the returned model, so holding the model is sufficient. Behaviour-neutral: reference model output is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move the SBML-to-internal-representation building out of ChasteSbmlModel into a ModelBuilder class: all the _format_* translators, _add_* data builders, ODE extraction, equation ordering and the name-conflict check, along with the collections they populate. ChasteSbmlModel becomes a thin orchestrator -- load the SBML, resolve id conflicts, build, populate the template context and render -- reading the built collections through ModelBuilder.template_data(). Behaviour-neutral: reference model output is byte-identical. The _check_name_conflicts tests move to test_model_builder.py, and a new test_chaste_sbml_model.py covers the orchestrator (write and the outputs property) end-to-end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Update the class docstring to describe ChasteSbmlModel's new role as an orchestrator over the loader, NameManager, ModelBuilder and CodeRenderer rather than a data holder, and correct the __init__ parameter docstrings to the actual sbml_file/model_name names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Correct _get_variable_type's return annotation (it returns a VarType, not bool) and add its missing :return:; parameterise the bare dict/set return types on template_data, _add_state_variable and NameManager._collect_ids; and return a copy from the outputs property so callers cannot mutate the model's internal state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Cover the builder's core SBML-to-internal translation directly (previously only exercised end-to-end by test_generation): state variables and their derivatives, parameters and reactions, amount/concentration conversions, one derivative equation per state variable, and event translation (trigger plus assignment), built from real reference SBML. Also drop a stale issue reference from a docstring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Replace the five-branch linear scan with a lookup against an id -> index map built once and cached (_build_variable_index). Reaction fluxes exposed as derived quantities are skipped so a reaction maps to its index among reactions, matching the previous dispatch. The map is only built while formatting events, after every variable has been added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Split the two largest ModelBuilder methods (~200 and ~185 lines) into focused helpers, and dedup the triplicated compartment dC/dt block behind _compartment_time_derivative. Behaviour-neutral: reference outputs unchanged and all 1191 generatable semantic cases render byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Introduce chaste_sbml/_records.py with typed dataclasses (StateVariable, Parameter, DerivedQuantity, Reaction, Function, Rule, InitialAssignment, Equation, LocalParameter, EventAssignment, Event) and have ModelBuilder build lists of these instead of ad-hoc dicts. Internal field access moves from record["field"] to record.field; the Jinja templates are unchanged (they fall back from record["field"] to record.field). Behaviour-neutral: 82 unit tests pass, reference outputs regenerate unchanged, and all 1191 generatable semantic cases render byte-identical to the dict-based output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Fix _add_equation's local_parameters description (now LocalParameter records, not names) and document _add_state_variable's StateVariable return value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The old enumerated list included py39 (contradicting requires-python >=3.10) and lagged new releases. Omitting the key entirely makes black infer an unreleased py315 upper bound from the open-ended requires-python, which trips its safety-check warning on every run. Pinning to the requires-python floor is a single sensible value with no drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated no new comments.

kwabenantim and others added 5 commits July 3, 2026 23:09

#35 Remove dead cpp code

ab8aeac

#35 Simplify SbmlMath

53397f8

#35 Add SbmlOdeSolverSetup

59501f9

#35 Tweak TysonNovak tolerance

26e554b

kwabenantim and others added 4 commits July 4, 2026 13:45

Merge branch 'develop' into 35-cpp-code-restructure

b79fbe8

kwabenantim requested a review from Copilot July 4, 2026 16:58

Copilot started reviewing on behalf of kwabenantim July 4, 2026 16:59 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

Comment thread chaste_sbml/chaste_sbml_model.py Outdated

Comment thread chaste_sbml/_names.py Outdated

kwabenantim requested a review from Copilot July 4, 2026 17:17

Copilot started reviewing on behalf of kwabenantim July 4, 2026 17:17 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

Comment thread chaste_sbml/chaste_sbml_model.py Outdated

Comment thread chaste_sbml/SbmlRefModels/src/SbmlOdeSolverSetup.hpp

kwabenantim and others added 3 commits July 4, 2026 18:46

kwabenantim requested a review from Copilot July 4, 2026 18:24

Copilot started reviewing on behalf of kwabenantim July 4, 2026 18:25 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

Comment thread chaste_sbml/chaste_sbml_model.py Outdated

Comment thread pyproject.toml

kwabenantim and others added 2 commits July 4, 2026 19:53

kwabenantim changed the title ~~cpp code restructure~~ code restructure Jul 4, 2026

kwabenantim and others added 4 commits July 4, 2026 20:29

kwabenantim requested a review from Copilot July 4, 2026 20:27

Copilot started reviewing on behalf of kwabenantim July 4, 2026 20:28 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

kwabenantim and others added 7 commits July 4, 2026 21:55

#35 Update docstrings stale after the record refactor

da90ed6

Fix _add_equation's local_parameters description (now LocalParameter records, not names) and document _add_state_variable's StateVariable return value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

kwabenantim requested a review from Copilot July 4, 2026 22:46

Copilot started reviewing on behalf of kwabenantim July 4, 2026 22:46 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

kwabenantim marked this pull request as ready for review July 5, 2026 02:59

kwabenantim linked an issue Jul 5, 2026 that may be closed by this pull request

Resolve variable name conflicts #35

Closed

kwabenantim merged commit 7231498 into develop Jul 5, 2026
15 of 16 checks passed

kwabenantim deleted the 35-cpp-code-restructure branch July 5, 2026 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

code restructure#38

code restructure#38
kwabenantim merged 27 commits into
developfrom
35-cpp-code-restructure

kwabenantim commented Jul 4, 2026

Uh oh!

codecov Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kwabenantim commented Jul 4, 2026

Uh oh!

codecov Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 4, 2026 •

edited

Loading