Skip to content

Add standalone reference tests for cellular ODE models #627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
samibismar wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add standalone reference tests for cellular ODE models #627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c0b4f62
Add Regazzoni active stress unit tests
samibismar Aug 7, 2026
011dab4
Add Nash-Panfilov active stress unit tests
samibismar Aug 11, 2026
57c0fd1
Refactor active stress unit test infrastructure
samibismar Aug 13, 2026
de9b938
Generalize active stress trajectory tests
samibismar Aug 19, 2026
2e8f8fc
Add IonicModel trajectory unit tests
samibismar Aug 22, 2026
afee211
Add Bueno-Orovio ionic trajectory tests
samibismar Aug 24, 2026
7d7d9f6
Add TTP ionic trajectory tests
samibismar Aug 25, 2026
dd479cd
Update ionic tests to use stimulated action potentials
samibismar Aug 26, 2026
e8915ef
Add reference generators for ODE model tests
samibismar Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Code/Source/solver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,14 @@ if(ENABLE_UNIT_TEST)
pthread # link pthread on ubuntu20
)

# Expose the path to the unit-test reference-data directory as a compile-time
# constant. CMAKE_CURRENT_SOURCE_DIR is Code/Source/solver/ regardless of
# where cmake is invoked, so the path is stable under the ExternalProject build.
# Named generically so future ionic-model tests can share the same directory.
Comment on lines +412 to +415

@michelebucelli michelebucelli Aug 29, 2026

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.

Some of these comments don't seem to be relevant. I suggest simplifying to:

Suggested change
# Expose the path to the unit-test reference-data directory as a compile-time
# constant. CMAKE_CURRENT_SOURCE_DIR is Code/Source/solver/ regardless of
# where cmake is invoked, so the path is stable under the ExternalProject build.
# Named generically so future ionic-model tests can share the same directory.
# Define the path to the unit-test reference-data directory as a preprocessor
# macro.

(Compile-time constants are a somewhat larger category than preprocessor macros, as they include constexpr declarations and some const declarations that the compiler can deduce to be compile-time constant. Preprocessor macro is more accurate here)

target_compile_definitions(run_all_unit_tests PRIVATE
UNIT_TEST_DATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/../../../tests/unitTests/reference_data"
)

# gtest_discover_tests(runUnitTest)
add_test(NAME all_unit_tests COMMAND run_all_unit_tests)

Expand Down
11 changes: 11 additions & 0 deletions Code/Source/solver/active_stress_nash_panfilov.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ class NashPanfilov : public ActiveStressODE {
add_parameter("calcium_crit", 1.0, required);
add_parameter("eta_T", 1.0, required);
}

/// Set a scalar parameter by name with full double precision.
///
/// @c double_parameters is @c protected in @ref ActiveStressModelParameters
/// and accessible here because this class is a derived class.
/// Throws @c std::out_of_range if @p label is not a registered parameter.
void set(const std::string &label, double value) {
auto &p = double_parameters.at(label);
p.value_ = value;
p.value_set_ = true;
Comment on lines +64 to +66

@michelebucelli michelebucelli Aug 29, 2026

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.

at throws an exception if the key is not found in the map, but this exception's message is quite generic and not very helpful in finding the source of the error. I suggest changing to something like this:

Suggested change
auto &p = double_parameters.at(label);
p.value_ = value;
p.value_set_ = true;
auto it = double_parameters.find(label);
svmp::check<svmp::FE::InvalidArgumentException>(
it != double_parameters.end(),
"Parameter " + label + " not found.");
it->second.value_ = value;
it->second.value_set_ = true;

}
Comment on lines +58 to +67

@michelebucelli michelebucelli Aug 29, 2026

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.

Might it be better to place this in some class higher up in the hierarchy, say ActiveStressModelParameters? Additionally, for symmetry with ActiveStressModelParameters::get_scalar, I'd rename this to set_scalar.

};

/**
Expand Down
2 changes: 2 additions & 0 deletions Code/Source/solver/ionic_bueno_orovio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class BuenoOrovio : public IonicModel {
add_parameter("k_so", {2.04580, 2., 2.1}, required);
add_parameter("u_so", {0.650, 0.65, 0.6}, required);
add_parameter("tau_s1", {2.73420, 2.7342, 2.7342}, required);
// svMP currently uses 2 ms for the M-cell tau_s2 default;
// Bueno-Orovio et al. (2008), Table 1, reports 4 ms.
Comment on lines +61 to +62

@michelebucelli michelebucelli Aug 29, 2026

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 suggest rephrasing this as:

Suggested change
// svMP currently uses 2 ms for the M-cell tau_s2 default;
// Bueno-Orovio et al. (2008), Table 1, reports 4 ms.
// Beware that the default here for the M-cell tau_s2 (X-th entry in
// the list below, 2 ms) is different from the value reported in
// Bueno-Orovio et al. (2008), Table 1, which is 4 ms.

(replace X-th with 2nd or 3rd depending on which is the right one).

Additionally, perhaps this should be moved to the general class documentation, so that it will be exposed in the Doxygen-generated files. I would suggest placing this in a @warning paragraph.

add_parameter("tau_s2", {16.0, 2., 2.}, required);
add_parameter("k_s", {2.09940, 2.0994, 2.0994}, required);
add_parameter("u_s", {0.90870, 0.9087, 0.9087}, required);
Expand Down
Loading
Loading