Skip to content

Add thirty models#13

Open
joeyshuttleworth wants to merge 89 commits into
masterfrom
fix_transition_matrix
Open

Add thirty models#13
joeyshuttleworth wants to merge 89 commits into
masterfrom
fix_transition_matrix

Conversation

@joeyshuttleworth

@joeyshuttleworth joeyshuttleworth commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator

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

@codecov

codecov Bot commented May 3, 2023

Copy link
Copy Markdown

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.

@MichaelClerx MichaelClerx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

Comment thread markov_builder/MarkovChain.py
Comment thread markov_builder/MarkovChain.py Outdated
Comment thread markov_builder/MarkovChain.py Outdated
@joeyshuttleworth joeyshuttleworth changed the title Fix transition matrix Add thirty models Jun 13, 2023
Joseph G. Shuttleworth added 2 commits December 16, 2025 19:02
@joeyshuttleworth

Copy link
Copy Markdown
Collaborator Author

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

mirams commented Dec 17, 2025

Copy link
Copy Markdown
Member

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?

Comment thread tests/models_myokit/model-30.mmt Outdated
'b2': ('p11 * exp(-p12*V)',),
}

open_state = 'O'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

O * Oh ?

Comment thread markov_builder/models/thirty_models/model1.py
Comment thread tests/test_30_models.py Outdated

class TestThirtyModels(unittest.TestCase):

def setUp(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread tests/test_30_models.py Outdated
Comment thread tests/test_30_models.py Outdated
Comment thread tests/test_30_models.py Outdated
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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread tests/test_30_models.py Outdated
Comment thread tests/test_30_models.py Outdated
Comment thread README.md
@MichaelClerx MichaelClerx requested review from MichaelClerx and removed request for MichaelClerx December 17, 2025 17:02
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.

3 participants