Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow directly setting presimulation and preequilibration parameters in model #931

Open
FFroehlich opened this issue Jan 31, 2020 · 12 comments
Assignees

Comments

@FFroehlich
Copy link
Member

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.

@dweindl
Copy link
Member

dweindl commented Feb 1, 2020

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.

@FFroehlich
Copy link
Member Author

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.

@dweindl
Copy link
Member

dweindl commented Feb 1, 2020

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
Copy link
Member

dweindl commented Feb 1, 2020

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.

@FFroehlich
Copy link
Member Author

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
Copy link
Member Author

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.

@dweindl
Copy link
Member

dweindl commented Feb 1, 2020

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.

@FFroehlich
Copy link
Member Author

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
Copy link
Member Author

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.

@dweindl
Copy link
Member

dweindl commented Feb 1, 2020

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

@FFroehlich
Copy link
Member Author

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.

@dweindl
Copy link
Member

dweindl commented Feb 3, 2020

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

dweindl added a commit that referenced this issue Feb 11, 2021
…1407)

This allows using Model without ExpData unless there really are measurements. Also avoids having to add any new member of ExpData also to Model and slims down Model a bit. Getters and setters for SimulationParameters members are still in Model, which is to preserve backwards compatibility for ExpData, which allowed unchecked access to the respective members. Further refactoring to follow.

Partially addresses #931 (moving ID / name functions is still missing).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants