Parsing optional statements in model interface.

[MichaelClerx]

Addresses #157. Fixes #105.

Significant rewrite of ModelInterface.modify_model(), based on #157 (comment)

• Before modifications begin, information from inputs, outputs, defines, clamp-tos, and var statements is collated and stored in ProtocolVariable objects.
• Where possible, ProtocolVariables are resolved to model variables, and sanity-checking is performed
  • To allow nesting, this is done slightly more leniently than before, e.g. multiple input declarations are allowed on the same variable, as long as they don't make conflicting demands
• Unit conversion of inputs is slightly rewritten, but more or less the same: I wanted to do outputs here too but this caused more bookkeeping due to transitive variables & the state_variable annotation
• Variable creation requires units, units must sometimes be inferred from the RHS, and this requires the RHS in sympy which requires all RHS variables to exist. To deal with this situation, creating and modifying variables is now performed in a single method, and in several passes. The system is deemed irresolvable if a single pass is unable to make any progress.

[MichaelClerx]

The s1_s2_noble test causes an error in cellmlmanip, where the to_unit becomes None but a conversion is still triggered, seems to go wrong here:

https://github.com/ModellingWebLab/cellmlmanip/blob/877918347d9d9e7be08692ba5bcc01780175eb71/cellmlmanip/units.py#L775-L778

if to_units is not None or was_converted:
  # Convert the result if required
  expr = expr.func(*new_args)
  expr, was_converted, actual_units = maybe_convert_expr(expr, was_converted, actual_units, to_units)

These lines do look wrong to me: maybe_convert_expr() can be called with to_units=None which seems to cause this error?

[MichaelClerx]

Stopped this from happening now, but the lines above still look dubious to me @jonc125 :D

[MichaelClerx]

I think the bit with the output variables can be tidied up further:

• make get_variables_transitively return variables + terms, apply this to outputs in collate to create ProtocolVariables for these outputs
• stop calling get_variables_transitively in _convert_output_units (now becomes a simple loop)
• store outputs better, and use this in protocol.py to pass to code generation, stop calling get_variables_transitively in code_generation.py
• presumably the rule is "If a non-optional output doesn't resolve directly, but does resolve transitively, then that's fine."?

[MichaelClerx]

Wondering if ModelInterface._expr and ModelInterface.merge can be simplified, and we no longer need to store all the base actions for inputs/outputs/optional etc.? (At least not separately)
Also duplicates would be fine in current code, so merge can be easier?

[jonc125]

This looks really good overall, and does tidy things up nicely. There are some instances of broken code suggesting that we're missing some important test coverage, and a couple of bits I'm not sure about. Plus a few cosmetic changes :)

[MichaelClerx]

Ready for final(?) review @jonc125

[MichaelClerx]

@jonc125 optional outputs?

[jonc125]

Looks good to me. I've just suggested a few minor tweaks.

[jonc125]

@jonc125 optional outputs?

I thought they would have been removed by this point? Maybe not. No tests failed though!

[MichaelClerx]

I think the protocol_variables list just gives all the information about variables specified in the protocol, including optional ones that couldn't be resolved. So probably if we add a test with an optional output that's also optional in post-processing this could fail?

[jonc125]

Quite possibly. In which case please do revert that change! (Perhaps with a comment explaining the situation.)

[MichaelClerx]

I agree it's a very ugly if-block 😅

[MichaelClerx]

Fixed now?

[jonc125]

Great!

[jonc125]

Huh, except that the assertion keeps tripping?

[MichaelClerx]

I forgot to set is_vector on the magic state_variable annotation. Fixed now!