Add thirty models #13

joeyshuttleworth · 2023-04-28T13:01:14Z

Description

Add models 1-11 and 30 from CardiacModelling/hergModels/tree/master/models_myokit. These models are tested by comparing Myokit Simulation output form the original .mmt files with those generated through the Markov_builder.

The newly added models make extensive use of the shared_variables_dict. The behaviour of parameterise_rates was modified to make it more convenient to set these variables to numerical values.

These changes exposed some issues with reversibility checking and transition matrix calculation which have now been fixed.

The new models can be easily generated by users and modified to include drug-trapping or additional states/parameters.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Testing

Testing is done automatically and codecov shows test coverage
This cannot be tested automatically

Documentation checklist

I have updated all documentation in the code where necessary.
I have checked spelling in all (new) comments and documentation.
I have added a note to RELEASE.md if relevant (new feature, breaking change, or notable bug fix).

…tch to draw_graph

…arkov-builder into fix_transition_matrix

codecov · 2023-05-03T11:02:03Z

Codecov Report

❌ Patch coverage is 97.91667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.30%. Comparing base (5672637) to head (93b454f).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
markov_builder/MarkovChain.py	92.04%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   89.71%   93.30%   +3.59%     
==========================================
  Files           5       22      +17     
  Lines         554      837     +283     
==========================================
+ Hits          497      781     +284     
+ Misses         57       56       -1

☔ View full report in Codecov by Sentry.
📢 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.

Add sundials install to workflow

MichaelClerx

Hi Joey,
Won't have time for an in-depth review (if you want that) till June or after.
Few small comments. And comparison 2 looks plain wrong?
Would plot an error for these 3 cases too (signal1 - signal2)

markov_builder/MarkovChain.py

…ctories

…s_class)

…arkov-builder into fix_transition_matrix

joeyshuttleworth · 2025-12-16T19:06:08Z

All 30 models have been added now. Some fixes have been applied to some of the .mmt files where it looks like there were errors

mirams · 2025-12-17T08:43:16Z

All 30 models have been added now. Some fixes have been applied to some of the .mmt files where it looks like there were errors

You'll have to show me those Joey! And do a pull request on the 30 models repo?

mirams · 2025-12-17T08:45:07Z

tests/models_myokit/model-30.mmt

@@ -1,4 +1,4 @@
-[[model]]
+model]]


mirams · 2025-12-17T10:41:48Z

markov_builder/models/thirty_models/model10.py

+        'b2': ('p11 * exp(-p12*V)',),
+    }
+
+    open_state = 'O'


markov_builder/models/thirty_models/model1.py

MichaelClerx · 2025-12-17T12:19:26Z

tests/test_30_models.py

+
+class TestThirtyModels(unittest.TestCase):
+
+    def setUp(self):


Did you mean setUpClass here?

The difference is that setUp gets called before every test_x method in your class, while setUpClass is called just once

MichaelClerx · 2025-12-17T12:22:54Z

tests/test_30_models.py

+        if not os.path.exists(comparison_plot_dir):
+            os.makedirs(comparison_plot_dir)
+
+        for i, model in zip(self.model_indices, self.models):


If you wanted, you could get fancy here and use subTest https://docs.python.org/3/library/unittest.html#subtests

There's also https://docs.pytest.org/en/stable/how-to/parametrize.html which seems to do exactly what we need here.

I'm not sure which way to go on this, but I'd definitely like to tidy these up so we can see which models are failing if there's a problem

Sure, but keep in mind pytest and unittest are two seperate packages

Yeah. I'm just trying to weigh up if switching to pytest is worth it for this. I think it is, because you can use the parametrize() decorator to register each model as a separate test, then do pytest --lf to only run those models that failed in the last run. Otherwise, running all the tests can take a few minutes.

MichaelClerx · 2025-12-17T12:23:32Z

tests/test_30_models.py

+        self.disconnected_models = [model_03, model_09, model_10, model_19,
+                                    model_20, model_26, model_27]
+
+        self.model_indices = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,


list(range(31))?

Alternatively, some test that you've (probably) listed the models right.
E.g. self.assertEqual(len(set(model_indices)), 31) and self.assertEqual(len(model_indices), len(models))

I suppose there's no need for the indices list anymore.

Before I had all 30 models (missing 15-29 or something), so I did this manually.

I think we can drop indices, and do enumerate with list(range(31))` as you suggest

MichaelClerx · 2025-12-17T12:25:27Z

tests/test_30_models.py

+        self.output_dir = test_output_dir
+        logging.info("outputting to " + test_output_dir)
+
+        self.models = [model_00, model_01, model_02, model_03, model_04,


[f'model_{i:02}' for i in self.model_indices] ?

MichaelClerx · 2025-12-17T12:27:10Z

tests/test_30_models.py

+                            f"model {model} is not connected")
+
+    def test_reversible(self):
+        for model in self.models:


Could so something slightly nicer here by using e.g. for model in self.models if ..., or using sets e.g. for model in set(self.models) - set(self.disconnected_models)
At least, I'd move the if statement to above the logging statement

MichaelClerx · 2025-12-17T12:27:50Z

tests/test_30_models.py

+            if i is None or model is None:
+                continue


Confused by this. You'd want the tests to fall over if they weren't configured right

README.md

Joseph added 12 commits October 24, 2022 12:13

Change default parameter handling

be57ad5

Remove unnecessary line from test

9d952fc

Add first 4 models. Add tests to generate .mmt and graph files

2297bf9

Add tests for connectedness as reversibility. Add models 4,5

ae770ab

Fix draw_graph handling of transition rates

0b6f9d8

Add more models

d502827

Fix get_transition_matrix. Fix reversibility check. Add show_html swi…

beacf8f

…tch to draw_graph

Merge branch 'master' into fix_transition_matrix

8904c6b

Add models 11 and 30. Remove duplicated test file

051a36f

Merge branch 'fix_transition_matrix' of github.com:CardiacModelling/M…

a4b97e2

…arkov-builder into fix_transition_matrix

Lint and isort

5310b55

Update test_parameterise_rates

57415dc

Joseph added 4 commits May 3, 2023 12:04

Add test_30_models.py

f199b6f

Add myokit files for testing

79f7295

Update pytest.yml

7e21caf

Add sundials install to workflow

Update pytest.yml

0afb97b

joeyshuttleworth requested a review from MichaelClerx May 3, 2023 13:34

MichaelClerx reviewed May 3, 2023

View reviewed changes

markov_builder/MarkovChain.py Show resolved Hide resolved

markov_builder/MarkovChain.py Outdated Show resolved Hide resolved

markov_builder/MarkovChain.py Outdated Show resolved Hide resolved

Joseph added 5 commits June 13, 2023 13:00

Make single quote use more consistent

bc4c4c9

Fix indentation. Distribute trajectories according to number of traje…

2d6c990

…ctories

Fix sympy depreciation warning (latex output)

17027f4

Add test for dataclass exception (must be subclass of state_attribute…

fe4f333

…s_class)

Merge branch 'fix_transition_matrix' of github.com:CardiacModelling/M…

23835be

…arkov-builder into fix_transition_matrix

joeyshuttleworth changed the title ~~Fix transition matrix~~ Add thirty models Jun 13, 2023

Joseph added 5 commits June 13, 2023 13:31

Raise TypeError not Exception

b6ef7da

Remove old, broken test output

215898f

Fix test_bad_dataclass_exception

de33024

Remove unnecessary checks on attribute dataclass

4d86220

Lint

fef0edf

Joseph G. Shuttleworth added 2 commits December 16, 2025 19:02

Lint

fc56ad3

Lint and isort

563a76e

mirams reviewed Dec 17, 2025

View reviewed changes

tests/models_myokit/model-30.mmt Outdated

@@ -1,4 +1,4 @@

[[model]]

model]]

Copy link

Member

mirams Dec 17, 2025

Choose a reason for hiding this comment

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

Typo?

mirams reviewed Dec 17, 2025

View reviewed changes

markov_builder/models/thirty_models/model10.py Outdated

'b2': ('p11 * exp(-p12*V)',),

}

open_state = 'O'

Copy link

Member

mirams Dec 17, 2025

Choose a reason for hiding this comment

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

O * Oh ?

mirams reviewed Dec 17, 2025

View reviewed changes

markov_builder/models/thirty_models/model1.py Show resolved Hide resolved

Revert ah<->bh swap

43bf6ef

MichaelClerx reviewed Dec 17, 2025

View reviewed changes

README.md Show resolved Hide resolved

Joseph G. Shuttleworth added 10 commits December 17, 2025 15:39

Fix mangled model file

2e8f31f

Remove open_state logic form 30 models

b43c386

Fix tests and use pytest

3e734d0

Split readme sentences onto newlines

756ced2

Lint, isort

fdbfa36

Remove old config

a61969c

move config to toml

fe63345

Tidy 30 new models

3f3a02d

Restore flake8 file

dc04819

isort

73ae5fc

MichaelClerx requested review from MichaelClerx and removed request for MichaelClerx December 17, 2025 17:02

Joseph G. Shuttleworth added 3 commits December 18, 2025 12:07

Fix model 0 parameter

e7ac78c

Eliminate correct state in test comparison

0631f15

Remove redundant 'open_state' vars

4874485

Add thirty models #13

Are you sure you want to change the base?

Add thirty models #13

Uh oh!

Conversation

joeyshuttleworth commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Testing

Documentation checklist

Uh oh!

codecov bot commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MichaelClerx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joeyshuttleworth commented Dec 16, 2025

Uh oh!

mirams commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joeyshuttleworth commented Apr 28, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading

mirams commented Dec 17, 2025 •

edited

Loading