ModellingWebLab / cellmlmanip

CellML loading and model equation manipulation
Other
3 stars 1 forks source link

Replace a variable's equation #118

Closed MichaelClerx closed 4 years ago

MichaelClerx commented 5 years ago

Given a variable, we need a method to set/replace its RHS

From FC, I can read equations (with variables specified as ontology terms) and units (using an FC unit syntax).

In a two-way communication approach, I could then:

Buuuut, that would mean fc talking to cellmlmanip through cg, so would probably prefer a one-way approach

So then we'd need to write some mini-spec for how cellmlmanip expects variable symbols and units?

MauriceHendrix commented 5 years ago

Given a variable You mean a sympy equation? I know you can't change lhs or rhs (i've tried) so it would have to be a replace I hink

jonc125 commented 5 years ago

But, doesn't fc have to talk to the cellmlmanip model anyway, to do modifications? Certainly I was assuming fc would talk to cellmlmanip first to parse and modify the model, then to cg to generate code? Not talk to cellmlmanip through cg?

MichaelClerx commented 5 years ago

Ah you're right it does

MichaelClerx commented 5 years ago

Given a variable You mean a sympy equation? I know you can't change lhs or rhs (i've tried) so it would have to be a replace I hink

I'd like to replace to entire RHS of a variable, so x = <expression_1> becomes x = <expression_2>. Or if x doesn't have an expression yet it would be x = ??? becomes x = <expression_2>. Would that be the same action in cellmlmanip or is there something I'm missing?

jonc125 commented 5 years ago

Also, I'd want what we build for fc to result in shared code (where sensible) with cellmlmanip's own parser implementation, which might mean some refactoring of the latter. For instance, we should be able to use Model.add_unit to add units defined in the protocol (though perhaps in a separate UnitStore, actually, would be better, to ensure the protocol doesn't accidentally use model units without defining them!). More likely, Model.add_variable for adding new variables defined just by the protocol, and we'll also need the ability to replace existing variables. Some of the logic in Parser._add_maths will probably be common, etc.

jonc125 commented 5 years ago

I'd like to replace to entire RHS of a variable, so x = <expression_1> becomes x = <expression_2>. Or if x doesn't have an expression yet it would be x = ??? becomes x = <expression_2>. Would that be the same action in cellmlmanip or is there something I'm missing?

I think for this you have to replace the whole equation in the Model's list (or add if it doesn't exist).

MichaelClerx commented 5 years ago

Also, I'd want what we build for fc to result in shared code (where sensible) with cellmlmanip's own parser implementation

So you'd want FC to generate (partial) CellML code and have cellmlmanip parse that?

jonc125 commented 5 years ago

Also, I'd want what we build for fc to result in shared code (where sensible) with cellmlmanip's own parser implementation

So you'd want FC to generate (partial) CellML code and have cellmlmanip parse that?

No no! I'd want the FC code to be able to use the same API on Model and friends as cellmlmanip's own parser does to build up the original model. Which might mean some refactoring of that API as it currently stands, as we identify opportunities.

skeating commented 5 years ago

(PLEASE correct me if this is not accurate)

  1. fc uses cellmlmanip to load a model; looks at the protocol and works out some stuff (parameters/outputs etc) then uses cg to create the relevant code that will then be used to 'run'.

  2. We anticipate users being able to use cellmlmanip as a standalone to play with editing models.

  3. We anticipate fc users wanting to read a model (possibly change it - aka Michael and this issue) and then cg and run.

If 3 is what we are aiming for them I strongly agree with Jonathan that any model manipulation be available and done using cellmlmanip. Judging by Maurice's requests/changes to cellmlmanip; cg uses cellmlmanip functionality to adjust the model - so this should be available for fc as well. It may be a case of providing wrapper functions in fc to provide the functionality from cellmlmanip directly.

mirams commented 5 years ago

I had in my mind that point 1. would look like:

  1. fc looks at the protocol and works out some stuff (parameters/outputs etc) then uses cellmlmanip to load a model and to manipulate equations to get inputs and outputs in correct units and throw away equations that aren't needed; cg simply prints these equations out in the required language that will then be used to 'run'.
MichaelClerx commented 5 years ago

Yes you're all right, I got a bit confused there. FC uses cellmlmanip to load the model, should then use cellmlmanip to modify it (this bit needs implementing), and then uses cg to create runnable code

MichaelClerx commented 5 years ago

OK, so I've got a string equation, let's say

"diff(oxmeta:membrane_voltage; oxmeta:time) = 1 :: mV_per_ms + 1 :: nA / oxmeta:membrane_capacitance"

and I want to insert this into an already parsed cellmlmanip model. I've got a parser for the string, so that's alright. Then I need to

Where number is a number or string, but units is the name of a unit, so I'd have to make sure these are preregistered first

Is that right @tamuri ?

jonc125 commented 5 years ago

You probably would want to pre-register units in this case, by parsing the protocol's units definitions before handling the model interface (this is what pycml does). Ideally you'd have a separate units store for protocol units, to check that only units defined in the protocol (or in the CellML standard set) are used.

MichaelClerx commented 5 years ago

Updated that list now with the item "Remove existing equation for dV/dt". Which is not strictly possible in CellML but is in practice

tamuri commented 5 years ago

Where number is a number or string, but units is the name of a unit, so I'd have to make sure these are preregistered first

Yes, units is the name of the unit that already exists in the unit registry because add_number and add_variable call units.get_quantity when setting up the metadata.

number has to be a sympy.Number (that's what the parser spits out) but that can be easily changed.

MichaelClerx commented 5 years ago

Jonathan any preference for either of these?

  1. get_equation_by_lhs() --> eq; remove_equation(eq); add_equation(new_eq) or
  2. replace_equation_for(lhs, new_eq) or simpler replace_equation_for(new_eq)
jonc125 commented 5 years ago

I suspect we'll want a remove_equation whichever; similarly for add_equation(eq)! But having a helper that calls these to do the work would be useful. replace_equation_for(lhs, new_eq) is clearer I think, even if somewhat redundant. We could do replace_equation(eq) though?

MichaelClerx commented 5 years ago

@jonc125 do we want the method to have the ability to turn states into non-states and vice-versa? It's no bother from a cellmlmanip point of view, but does give users more power to mess things up

jonc125 commented 5 years ago

Yes, for WL protocols this does need to be able to change the 'type' of variables by changing their definitions. For instance applying a voltage clamp...

MichaelClerx commented 5 years ago

Two final things:

  1. Just noticed there's an add_equation that now does the same as set_equation but without the checking for duplicates. Is it ok for me to remove this?
  2. Can we change .equations to ._equations to stop users from messing with it? (which requires them to then invalidate the graph, see #128 )
jonc125 commented 5 years ago
  1. With all the discussion last night I've lost track of what set_equation or replace_equation we've ended up with! To me set_equation feels like a really bizarre method name though - I'm not sure what I'd expect it to do! I'd prefer the pair of add and replace because that seems clearer what they're doing.
  2. Yep, that's fine. If users need read access to the list we can add a read-only property version.
MauriceHendrix commented 5 years ago

I don't mind add and replace but what I really don't want is a replace which takes just 1 argument that just feels wrong!

MichaelClerx commented 5 years ago

So we want

def replace_equation(lhs, rhs):
    # Replaces eq for var named in lhs with sp.Eq(lhs, rhs)
    # (Potentially changes states to non-states and vice-versa)
    # Complains if no equation for lhs exists

def add_equation(lhs, rhs):
    # Adds eq for var named in lhs
    # (Determines whether this var is a state, constant, etc.)
    # Complains if a previous equation for lhs exists

?

MichaelClerx commented 5 years ago

Yes? No? @MauriceHendrix @jonc125 ?

jonc125 commented 5 years ago

I'd keep the existing Model.add_equation(eq), as a complete equation is what you get from parsing and what you add to the list, so it's more straightforward.

Personally I like replace_equation(eq) because that's what the logic is from the FC point of view, but @MauriceHendrix is dead against it :) And I can see why as it's a fairly FC-specific use case. So two possible alternatives:

  1. Model.replace_equation_for(lhs, eq) - seems fairly intuitive to me, but a bit more work for callers to split out the equation they've already got into the required arguments.
  2. Model.remove_equation(eq) - and let FC have a replace_equation(eq) method internally. Makes the cellmlmanip API more self-consistent, at the expense of a bit more code in FC.