Add thirty models

[joeyshuttleworth]

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 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]

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)

[joeyshuttleworth]

We don't have all 30 yet models yet, but there are some important changes to the transition-rate matrix calculations that it would be good to merge in

[MichaelClerx]

Initial review on the start of the main class.
Some patterns are emerging that you can fix elsewhere before I continue

• Try to keep the API minimal, providing access only where needed and using private variables to help the user know what to ignore. The point of a good class is that it hides details from the user
• Don't use logging everywhere in case you want to debug, some day. Avoid the need for debugging by having good tests. Add debug statements only when actually debugging and remove if needed. In Python this code is always run (unlike e.g. C macros), so it will slow things down for no good reason
• Stop calling things x_dict
• Describe all arguments in the class/function docstring.
• If there's any relations between argument (e.g. x is only checked if y has a certain value) then (1) see if you can design it better so that this doesn't occur; (2) if you really can't then check and raise ValueErrors when the user does it wrong; (3) document exactly what the allowable input is

[MichaelClerx]

Since these graphs don't seem to have any kind of typing, you'll need a big comment above to explain what goes in the graph, what is stored in its nodes, etc.

Also, I'd strongly recommond making as many variables as you can private. It says to the user of this class "You don't need to know how this bit works, leave this to me". If they are public, then define them at class level first and provide a docstring for each

[MichaelClerx]

When deciding what to hide from the user, err on the side of hiding absolute everything. It's easy to provide access later. Initially, only write for the use case you know. If more are found later it's an easy job to provide get/set

[joeyshuttleworth]

Will set things to private in a separate PR. Want to make a nice version available without breaking anything external too much (my other projects)

[MichaelClerx]

The docstring is not very clear. How is markov chain also a compartmental model?

The "using networkx" bit is exactly what a good class should hide from the user. If I'm using this class I don't want to know how you implemented it, I just need to know how to use it.

Instead of saying it has various functions and tests, tell me the main ones. Give an example of how I use it

[MichaelClerx]

The () after the classname are unnecessary and best avoided, as it makes it look like a function

[joeyshuttleworth]

First comment is fair, and I've taken it out of the docstring. But I think it might be worth mentioning that this could be used as a way of generating a suitable networkx graph so it can be manipulated / plotted / outputted with other tools. But admittedly, I didn't end up doing any of that.

[MichaelClerx]

I get that. But it's generally a good guideline to only implement what you need. One good reason (beyond the simplicity and time saving mentioned below) is that it helps you test things: if you add a function you never use, you won't notice if it's broken. Much easier to just add it when you need to

https://dev.to/alisamir/yagni-you-arent-gonna-need-it-a-key-principle-in-software-development-167m

[joeyshuttleworth]

That's a good point. A lot of things in this package started out as a sort of prototype, so it'd be worth reevaluating what's actually needed

[MichaelClerx]

Raise an exception or if you reeeeeeally insist on warning but continuing use warnings.warn()

The logging module is for big server applications like apache or a DB or mail server or something.

If you want debug statements just use print and comment them out when not debugging (or simply remove them - again don't program for contingencies and eventualities, deal with them if they happen and add tests for the use cases you expect

[joeyshuttleworth]

I've found logging useful when importing markov builder into other projects. It can be help when debugging code in those projects and I think it can be disabled and the levels can be set externally too. Without this, I might have to have a local version of Markov builder and be editing in print statements.

Not too bothered, but I think it'd be convenient (for me at least) to keep them in.

[MichaelClerx]

If you were going very "proper" software dev here, then you'd say you want a "separation of concerns" which says that you would never want to know about markov builder internals from another project, because (1) there shouldn't be any bugs (full unit tests) and (2) the API and documentation should guide your use of the markov builder, not the implementation of that API

For this particular example, I think you want either

1. An exception, because this shouldn't happen so the program should stop, or
2. An exception, which the user code should catch and use to update whatever needs updating (I don't think this is likely here), or
3. A warning (warnings.warn('This thing happened but code will keep running anyway'))

A typical use case for a warning would be something like you're reading a file and it's in a newer format than was tested for so you let the user know that it will probably work but is untested. Or you're doing some fancy matrix thing that isn't 100% necessary and it fails so you can continue with an alternative strategy.

In this case I'm pretty sure you want an exception: The user's code is wrong or else this shouldn't happen, so stop

[MichaelClerx]

In particular cause it's a TypeError you're catching here. So its looks like it definitely the user doing it wrong, not markov builder, so you want to say "I can't guarantee the output, because this needs to fixed on your side"?

Worth even testing that they're both set before calling numpy?

[joeyshuttleworth]

I should have mentioned - this function has been changed now to raise a ValueError in this case.

markov-builder/markov_builder/MarkovChain.py

Line 518 in fb54457

def get_equilibrium_distribution(self, param_dict: dict = {}) -> Tuple[List[str], np.array]:

    def get_equilibrium_distribution(self, param_dict: dict = {}) -> Tuple[List[str], np.array]:
        """Compute the equilibrium distribution of the CTMC for the specified transition rate values

        :param param_dict: A dictionary specifying the values of each transition rate
        :return: A 2-tuple describing equilibrium distribution and labels defines which entry relates to which state

        :raises ValueError: If not every necessary parameter is defined in param_dict

        """


        A, B = self.eliminate_state_from_transition_matrix(use_parameters=True)
        labels = self.graph.nodes()
        vars_used = [*A.free_symbols, *B.free_symbols]


        for _var in vars_used:
            if str(_var) not in param_dict:
                raise ValueError(f"Error evaluating equilibrium distribution {_var}\n"
                                 f"A={A}\nB={B}\nparams={param_dict}\n")


        ss = -np.array(A.LUsolve(B).evalf(subs=param_dict)).astype(np.float64)
        ss = np.append(ss, 1 - ss.sum())
        return labels, ss

I think there was a possibility of a TypeError if you didn't provide a value for some of the parameters. In that case, it tries to evaluate a sympy Symbol as a float.

[joeyshuttleworth]

But there are still some debug messages elsewhere