Skip to content

Add standalone reference tests for cellular ODE models - #627

Open
samibismar wants to merge 19 commits into
SimVascular:mainfrom
samibismar:feature-ode-unit-test
Open

Add standalone reference tests for cellular ODE models#627
samibismar wants to merge 19 commits into
SimVascular:mainfrom
samibismar:feature-ode-unit-test

Conversation

@samibismar

Copy link
Copy Markdown
Contributor

Resolves #597.

Current situation

The existing regression tests for cellular ODE models use reference solutions generated by svMultiPhysics itself and are generally coupled to larger 3D problems.

This PR adds standalone unit tests for ionic and active-stress models using independently generated reference trajectories.

Release Notes

  • Added shared trajectory-test infrastructure for IonicModel and ActiveStress.
  • Added full-state reference tests for:
    • Aliev-Panfilov
    • FitzHugh-Nagumo
    • Bueno-Orovio EPI, ENDO, and M
    • ten Tusscher-Panfilov 2006 EPI, ENDO, and M
    • Nash-Panfilov
    • Regazzoni/RDQ20-MF
  • Ionic-model tests start from their initial/rest states, apply a time-dependent stimulus, and verify the resulting transient.
  • Active-stress tests use prescribed calcium and mechanical inputs to verify the full model-state trajectory.
  • Added reference CSVs under tests/unitTests/reference_data/.
  • Added independent reference generators and a verifier under tests/unitTests/reference_generators/.

Testing

  • Full unit-test suite passes: 252/252.
  • Reference-data verification passes for all 10 trajectories.

Code of Conduct & Contributing Guidelines

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@samibismar

Copy link
Copy Markdown
Contributor Author

The Regazzoni reference generator currently uses the authors’ C++ implementation at a pinned commit, so regenerating it requires a local clone of cardiac-activation together with Eigen, Boost, and a C++ compiler. I chose this rather than maintaining another transcription of RDQ20-MF, since it keeps the reference tied directly to the authors’ implementation.

I did consider making it more self-contained, either by optionally fetching the pinned source automatically or by writing an independent Python version similar to the TP06 generator. I also wasn’t sure whether bundling or automatically fetching an external reference implementation is something we want to do in svMultiPhysics, so I didn’t want to make that choice without asking. I’d be happy to change the workflow if you think a more self-contained approach would be preferable.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.91635% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.20%. Comparing base (9ff6e43) to head (97f7916).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...s/active_stress_tests/active_stress_test_helpers.h 92.15% 16 Missing ⚠️
...Tests/ionic_model_tests/ionic_model_test_helpers.h 91.83% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
+ Coverage   72.86%   73.20%   +0.34%     
==========================================
  Files         258      266       +8     
  Lines       39500    40003     +503     
  Branches     6730     6757      +27     
==========================================
+ Hits        28780    29284     +504     
+ Misses      10477    10476       -1     
  Partials      243      243              

☔ 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.

@michelebucelli michelebucelli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @samibismar! This is a very useful contribution. I have left some comments below.

Comment thread Code/Source/solver/active_stress_nash_panfilov.h Outdated
Comment thread Code/Source/solver/active_stress_nash_panfilov.h Outdated
Comment thread Code/Source/solver/CMakeLists.txt Outdated
Comment thread Code/Source/solver/ionic_bueno_orovio.h Outdated
Comment thread tests/unitTests/active_stress_tests/active_stress_test_helpers.h Outdated
Comment thread tests/unitTests/ionic_model_tests/ionic_model_test_helpers.h
Comment thread tests/unitTests/reference_data/active_stress_nash_panfilov_twitch.csv Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the generation of reference solutions should be delegated to trusted third-parties, if possible. Otherwise there's a risk that we are simply comparing two different solvers made by the same people, and both of them might contain the same errors.

I can see that this particular flavor of the Nash-Panfilov model is not the vanilla one (as in the original paper), but it is the slightly modified version from Goktepe and Kuhl, and so the CellML-generated code might not match exactly.

Still, I suggest taking that CellML code (I think it's this one, but do double-check) and applying the modifications there, rather than rewriting the thing from scratch.

A similar suggestion applies to other files as appropriate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also beware that the copyright of CellML-generated code probably still lies with the Cell-ML authors, and so the generated scripts should not contain the header about copyright being held by Stanford University etc.

The CellML model page (at least on the new website) has some licensing and authorship information, and that should be followed in the scripts I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you! I looked into the other reference generators before changing anything. The same general issue applies to all of them: the current generators run independently of svMultiPhysics, but the model equations were still transcribed locally.

I found the following external implementations and the minimum adaptations that would be needed:

Model Trusted external base Minimal adaptations needed Plan
Nash–Panfilov Physiome Nash–Panfilov 2004 CellML Replace the original piecewise, voltage-driven active-tension law with the Göktepe–Kuhl Gompertz form used by svMP; replace voltage activation with intracellular calcium; apply the existing svMP calibration and FE test protocol. Use the CellML-generated implementation as the trusted starting point and make only the explicit changes needed to reproduce the svMP Nash variant.
FitzHugh–Nagumo Published Physiome FitzHugh 1961 CellML The equations already match after the parameter/time transformation. Apply the svMP parameters and time scaling, then use the existing FE test protocol. Replace the handwritten FN equations with the CellML-generated equations and keep only the svMP-specific test protocol locally.
TP06 EPI/ENDO/M Physiome TP06 CellML: EPI / ENDO / M Keep the generated TP06 equations and phenotype parameters, but use the test’s FE/Rush–Larsen stepping, stimulus convention, phenotype initial states, and state ordering. Replace the handwritten TP06 equations with the CellML-generated equations while keeping the svMP-specific numerical/test protocol.
Aliev–Panfilov Pigreads aliev1996simple.cl Adapt the original AP formulation to the Göktepe–Kuhl/svMP form: separate the alpha and b parameter roles, apply the svMP voltage/time scaling, initial state, and stimulus convention, remove diffusion, and use the existing FE test protocol. Use Pigreads as the trusted external starting implementation if you think it is an appropriate source.
Bueno–Orovio EPI/ENDO/M openCARP Bueno.model The equations already match closely. Apply the EPI/ENDO/M parameter sets, intentionally retain svMP’s M-cell tau_s2 = 2 ms value instead of the published 4 ms, and use the existing FE test protocol. Use the pinned openCARP implementation as the trusted external oracle, but keep openCARP external rather than copying its implementation into svMultiPhysics.

I also checked the source licensing/authorship because of your follow-up comment. The Physiome sources have explicit attribution requirements, Pigreads is MIT-licensed, and openCARP uses a more restrictive academic license.

Does the Nash/FN/TP06 plan look reasonable to you?

For Aliev–Panfilov, do you think Pigreads is a sufficiently trustworthy external source for this purpose? I also found the Physiome AP workspace, but it is a poorer match to the svMP formulation and would require more adaptation.

For Bueno–Orovio, openCARP is a much closer match than the older Physiome bueno_2007 model, but its license is more restrictive. Would you prefer that I handle it more like Regazzoni—document the exact external source/version used to generate the committed reference trajectory rather than include/adapt the implementation inside svMultiPhysics—or use another approach you think would be better?

# SPDX-FileCopyrightText: Copyright (c) Stanford University, The Regents of the
# University of California, and others. SPDX-License-Identifier: BSD-3-Clause

"""Regenerate and verify all unit-test reference trajectories."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the verification doing here? I suggest expanding the documentation to explain this more clearly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds standalone, deterministic 0D trajectory unit tests for multiple IonicModel and ActiveStress implementations, comparing svMultiPhysics time-stepping against independently generated reference CSV trajectories (rather than references produced by svMultiPhysics itself).

Changes:

  • Added shared trajectory-test helpers for IonicModel and ActiveStress, including CSV parsing and checkpoint-based comparisons.
  • Added independent Python reference generators + a verifier script, plus committed reference CSVs for multiple ionic and active-stress models.
  • Wired unit-test builds to locate reference CSVs via a UNIT_TEST_DATA_DIR compile definition.

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
tests/unitTests/reference_generators/verify_reference_data.py Adds a script to (re)run in-repo generators twice for determinism and byte-compare against committed reference CSVs.
tests/unitTests/reference_generators/README.md Documents generator goals, requirements, and how to run the verifier.
tests/unitTests/reference_generators/ionic_model/ten_tusscher_panfilov/ttp_model.py Adds an independent TP06 model implementation used by the TP06 generator.
tests/unitTests/reference_generators/ionic_model/ten_tusscher_panfilov/README.md Documents TP06 provenance, protocol, and state ordering.
tests/unitTests/reference_generators/ionic_model/ten_tusscher_panfilov/generate_ttp.py Generates canonical TP06 checkpoint trajectories for epi/endo/m.
tests/unitTests/reference_generators/ionic_model/fitzhugh_nagumo/README.md Documents FitzHugh–Nagumo provenance and protocol.
tests/unitTests/reference_generators/ionic_model/fitzhugh_nagumo/generate_fitzhugh_nagumo.py Generates canonical FitzHugh–Nagumo checkpoint trajectory.
tests/unitTests/reference_generators/ionic_model/bueno_orovio/README.md Documents Bueno–Orovio provenance, protocol, and the M-cell tau_s2 note.
tests/unitTests/reference_generators/ionic_model/bueno_orovio/generate_bueno_orovio.py Generates canonical Bueno–Orovio checkpoint trajectories for epi/endo/m.
tests/unitTests/reference_generators/ionic_model/aliev_panfilov/README.md Documents Aliev–Panfilov provenance and protocol.
tests/unitTests/reference_generators/ionic_model/aliev_panfilov/generate_aliev_panfilov.py Generates canonical Aliev–Panfilov checkpoint trajectory.
tests/unitTests/reference_generators/active_stress/regazzoni/README.md Documents how the external Regazzoni reference CSV was produced and its conventions.
tests/unitTests/reference_generators/active_stress/nash_panfilov/README.md Documents Nash–Panfilov provenance and protocol.
tests/unitTests/reference_generators/active_stress/nash_panfilov/generate_nash_panfilov.py Generates canonical Nash–Panfilov twitch reference CSV.
tests/unitTests/reference_data/ionic_ttp_m_trajectory.csv Adds TP06 M-cell checkpoint reference data.
tests/unitTests/reference_data/ionic_ttp_epi_trajectory.csv Adds TP06 EPI checkpoint reference data.
tests/unitTests/reference_data/ionic_ttp_endo_trajectory.csv Adds TP06 ENDO checkpoint reference data.
tests/unitTests/reference_data/ionic_fitzhugh_nagumo_fe_trajectory.csv Adds FitzHugh–Nagumo checkpoint reference data.
tests/unitTests/reference_data/ionic_bueno_orovio_m_trajectory.csv Adds Bueno–Orovio M-cell checkpoint reference data.
tests/unitTests/reference_data/ionic_bueno_orovio_epi_trajectory.csv Adds Bueno–Orovio EPI checkpoint reference data.
tests/unitTests/reference_data/ionic_bueno_orovio_endo_trajectory.csv Adds Bueno–Orovio ENDO checkpoint reference data.
tests/unitTests/reference_data/ionic_aliev_panfilov_stimulated_trajectory.csv Adds Aliev–Panfilov checkpoint reference data.
tests/unitTests/reference_data/active_stress_regazzoni_twitch.csv Adds Regazzoni twitch reference data (externally generated).
tests/unitTests/reference_data/active_stress_nash_panfilov_twitch.csv Adds Nash–Panfilov twitch reference data.
tests/unitTests/ionic_model_tests/test_ionic_ttp.cpp Adds standalone TP06 EPI/ENDO/M trajectory unit tests using the shared helper.
tests/unitTests/ionic_model_tests/test_ionic_fitzhugh_nagumo.cpp Adds standalone FitzHugh–Nagumo trajectory unit test using the shared helper.
tests/unitTests/ionic_model_tests/test_ionic_bueno_orovio.cpp Adds standalone Bueno–Orovio EPI/ENDO/M trajectory unit tests using the shared helper.
tests/unitTests/ionic_model_tests/test_ionic_aliev_panfilov.cpp Adds standalone Aliev–Panfilov trajectory unit test using the shared helper.
tests/unitTests/ionic_model_tests/ionic_model_test_helpers.h Adds shared helper to run standalone ionic-model trajectories and compare to reference CSV checkpoints.
tests/unitTests/active_stress_tests/test_active_stress_regazzoni.cpp Adds standalone Regazzoni active-stress trajectory test using the shared helper.
tests/unitTests/active_stress_tests/test_active_stress_nash_panfilov.cpp Adds standalone Nash–Panfilov active-stress trajectory test using the shared helper.
tests/unitTests/active_stress_tests/active_stress_test_helpers.h Adds shared helper to run a standardized twitch protocol and compare to reference CSV checkpoints.
Code/Source/solver/Parameters.h Adds ActiveStressModelParameters::set_scalar for programmatic scalar parameter configuration.
Code/Source/solver/ionic_ttp.h Adds documentation clarifying zone-id effects on TP06 s-gate kinetics.
Code/Source/solver/ionic_fitzhugh_nagumo.h Improves FitzHugh–Nagumo reference documentation and parameterization context.
Code/Source/solver/ionic_bueno_orovio.h Adds warning about the M-cell tau_s2 value divergence from the paper.
Code/Source/solver/ionic_aliev_panfilov.h Expands Aliev–Panfilov documentation and references.
Code/Source/solver/CMakeLists.txt Defines UNIT_TEST_DATA_DIR compile-time macro for unit-test reference CSV lookup.
Code/Source/solver/active_stress_nash_panfilov.h Adds documentation clarifying calcium-driven formulation mapping to published voltage-driven model.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1447 to +1449
parameter->second.value_ = value;
parameter->second.value_set_ = true;
}
Comment on lines +15 to +18
void set_parameter_value(const std::string &name, double value)
{
parameters.at(name).set(name, true, value);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Verification and testing of cellular ODE-based models

3 participants