AMICI icon indicating copy to clipboard operation
AMICI copied to clipboard

Allow directly setting presimulation and preequilibration parameters in model

Open FFroehlich opened this issue 4 years ago • 12 comments

Having to create and Expdata instance for is often a bit cumbersome and this may also ease the programmatic creation of ExpData using the model constructor.

FFroehlich avatar Jan 31 '20 17:01 FFroehlich

What about just giving Model an ExpData member instead of replicating all the ExpData members? This would also get rid of the need for the ConditionContext class, that I don't really like, due to the side effects in the destructor.

dweindl avatar Feb 01 '20 12:02 dweindl

I see your point. My Primary issue is that interactive simulations are quite cumbersome at the moment. ExpData doesnt have any ByName setters or getters and the most of the members are immutable tuples, which effectively means that changing a single value typically involves 3-4 lines of code compared to a single call when setting variables directly in the model.

FFroehlich avatar Feb 01 '20 16:02 FFroehlich

Yeah, could be more comfortable, agreed.

ExpData doesnt have any ByName setters or getters and the most of the members are immutable tuples

Let's add them? Pull the ids/names and some of the dimensions out of Model into some ModelInfo. Both Model and ExpData can have one of those then. The getters/setters we can move to ExpData, so that you do model->edata->setParameterById(), or model->edata->setMeasurementForObservable(id, values). Having all those things both as part of Model and ExpData feels wrong and like a maintenance struggle.

Having names and IDs in a separate object would also be quite convenient for tying that info onto ReturnData. Allowing for rdata->getResiduals(observableId, time) could also be quite nice...

dweindl avatar Feb 01 '20 17:02 dweindl

Maybe off-topic with respect to setting parameters, but regarding convenient interactive use of ExpData: If find it currently pretty inconvenient that I am expected to decide on the number of time points / replicates during the creation of ExpData. Would be great if one could just append data for a given observable for arbitrary timepoints.

dweindl avatar Feb 01 '20 17:02 dweindl

Yeah, could be more comfortable, agreed.

ExpData doesnt have any ByName setters or getters and the most of the members are immutable tuples

Let's add them? Pull the ids/names and some of the dimensions out of Model into some ModelInfo. Both Model and ExpData can have one of those then. The getters/setters we can move to ExpData, so that you do model->edata->setParameterById(), or model->edata->setMeasurementForObservable(id, values). Having all those things both as part of Model and ExpData feels wrong and like a maintenance struggle.

Hmm adding them to ExpData is also duplicating code. How about we permanently add an ExpData instance to model where all parameters/fixpars are set and remove respective members from Model. This makes the ExpData(model) constructor way simpler to maintain (currently fields such as params and initial states are not carried over, may want to add extra fields to control behaviour and guarantee some sort of backwards compatibility).

Having names and IDs in a separate object would also be quite convenient for tying that info onto ReturnData. Allowing for rdata->getResiduals(observableId, time) could also be quite nice...

Yes, ReturnData will have to be refactored at some point ...

FFroehlich avatar Feb 01 '20 17:02 FFroehlich

Maybe off-topic with respect to setting parameters, but regarding convenient interactive use of ExpData: If find it currently pretty inconvenient that I am expected to decide on the number of time points / replicates during the creation of ExpData. Would be great if one could just append data for a given observable for arbitrary timepoints.

Wait, I though that should already be possible. I may need to set timepoints first before you are able to set new observables. With different dimensions.

FFroehlich avatar Feb 01 '20 17:02 FFroehlich

How about we permanently add an ExpData instance to model where all parameters/fixpars are set and remove respective members from Model.

Exactly what I meant. So 👍 .

Wait, I though that should already be possible. I may need to set timepoints first before you are able to set new observables. With different dimensions.

I may have missed it. I know that I can change timepoints and the other members will grow / shrink accordingly, but I thought I need to re-set all data. So appending is afaik not possible.

dweindl avatar Feb 01 '20 17:02 dweindl

I may have missed it. I know that I can change timepoints and the other members will grow / shrink accordingly, but I thought I need to re-set all data. So appending is afaik not possible.

Oh didn't read the appending part, should be straightforward to implement though.

FFroehlich avatar Feb 01 '20 19:02 FFroehlich

How about we permanently add an ExpData instance to model where all parameters/fixpars are set and remove respective members from Model.

Exactly what I meant. So 👍 .

Hmm, I think I wasn't specfic enough. I thought about keeping the setter/getter routines as model method that edit the value in the attached ExpData withouth explicitely adding the ModelInfo Instance, but its probably better to have model methods that model->setParameterById(...) that just propagate args to model->edata->setParameterById().

Should we enforce passing a model instance when instantiating ExpData then? Currently multiple models can reuse ExpData as long as dimensions are consistent, by adding names, these should also be consistent, which would be a bit more expensive to check everytime we pass an instance.

FFroehlich avatar Feb 01 '20 19:02 FFroehlich

Okay, then it's not what I meant. I would avoid having those getters/setters twice. So if we want to keep them directly on the Model instance (which would be convenient in terms of compatibility), then I would implement them in ExpData and have Model inherit from ExpData instead of owning one. Not great design, but maybe a compromise.

Should we enforce passing a model instance when instantiating ExpData then? Currently multiple models can reuse ExpData as long as dimensions are consistent, by adding names, these should also be consistent, which would be a bit more expensive to check everytime we pass an instance.

Maybe not enforce, but establish Model::createExpData() as default way of creating new instances. Yes, would be good to also check the IDs. For most models, that shouldn't get crazily expensive. May rather safe some time by preventing some mistakes...

dweindl avatar Feb 01 '20 19:02 dweindl

Okay, then it's not what I meant. I would avoid having those getters/setters twice. So if we want to keep them directly on the Model instance (which would be convenient in terms of compatibility), then I would implement them in ExpData and have Model inherit from ExpData instead of owning one. Not great design, but maybe a compromise.

What about creating a seperate class SimData that carries parameter scale, parameters, timepoints and fixedParameters for preequilibration/presimulation/simulation that has all the getter/setter methods including the byName/byId + regex variants. Both model and ExpData would inherit from that class.

FFroehlich avatar Feb 03 '20 16:02 FFroehlich

Sounds good. Maybe something like SimulationParameters? Sounds less like model outputs. But I can also go with SimData.

dweindl avatar Feb 03 '20 16:02 dweindl