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

generalise simulation, include sensititivies #3834

Closed
martinjrobins opened this issue Feb 23, 2024 · 7 comments · Fixed by #4415
Closed

generalise simulation, include sensititivies #3834

martinjrobins opened this issue Feb 23, 2024 · 7 comments · Fixed by #4415
Labels
feature refactor refactoring existing code

Comments

@martinjrobins
Copy link
Contributor

martinjrobins commented Feb 23, 2024

Description

at the moment simulation is a specific to the battery models and experiment class within pybamm. I'd like to propose a simpler, more general purpose mechanism so that a sequence of pybamm models can be solved (in sequence), with the next model initialised from the previous (including sensitivities). This doesn't have to be anything fancy, just a way to reinitialise a model from the solution of another model (although you could also have a base simulation class that works for a general pybamm model). I'd imagine that the simulation class could use this to simplify some of its logic as well, and it would mean that sensitivities could be used with experiments

Motivation

This was motivated by #3804
and #3176

Possible Implementation

No response

Additional context

No response

@martinjrobins martinjrobins changed the title generalise simulation generalise simulation, include sensititivies Feb 23, 2024
@martinjrobins martinjrobins added the refactor refactoring existing code label Feb 23, 2024
@valentinsulzer
Copy link
Member

Sounds good! I agree the Simulation class should not have anything battery-specific.

How does the concept of an Experiment generalize beyond battery models? Specifically, the way to "solve pybamm models in sequence" at the moment is via the Experiment. Should we have a generic Experiment class to generalize this? Or allow passing in a list of models instead?
I'm currently working on moving the logic for experiments to the Experiment / Step class, mainly motivated by allowing custom experiments, which will be a first step towards generalizing the Simulation class.

Regarding sensitivities, how do we deal with events? I guess it's ok with forward sensitivities but difficult/impossible with adjoints.

@brosaplanella
Copy link
Member

Agree, that's a great idea! I think a list of models, if possible, might be easier to start with, as it would be hard to abstract what a generic Experiment is without other examples. At the moment, it is just different models ran sequentially with certain events that dictate on when to finish a simulation, so it could all be encapsulated in a list of models. We can always think how to generalise an experiment or to write other Step.

@valentinsulzer
Copy link
Member

Passing a list of models for the Simulation object might be confusing though, in that case I would suggest an entirely different class or a separate API like Simulation.from_list to load using a list of models instead of one model + an experiment

@martinjrobins
Copy link
Contributor Author

I think you need more than a list of models, you need:

  • a list of (discretised) models
  • a finish condition for model a (expression that has a zero crossing when the model finishes?, or list of events?)
  • a map between state vector for model a and the state vector for model b (another expression tree?)
  • anything else?

I don't know how you would do adjoints, but we don't do this atm anyway. We only do forward sensitivities

@valentinsulzer
Copy link
Member

a map between state vector for model a and the state vector for model b (another expression tree?)

This is currently handled by model.set_initial_conditions_from, which uses the Variable.name and the name of the variable in the variables list to match up the variables

How different would the models in the list be? Do you have a particular use case in mind?

@martinjrobins
Copy link
Contributor Author

a map between state vector for model a and the state vector for model b (another expression tree?)

This is currently handled by model.set_initial_conditions_from, which uses the Variable.name and the name of the variable in the variables list to match up the variables

How different would the models in the list be? Do you have a particular use case

nope, this sounds perfect, happy to use this

@valentinsulzer
Copy link
Member

It sounds like a more generic Experiment / Step might be the way to go then, with the Experiment / Step handling both how each step runs and when it terminates. After #3530 is merged in, the Step class will be responsible for modifying the model. This can easily be generalized beyond battery models, as long as each model being subsequently solved is not too different.

I'm not sure we need to support the use case of solving completely different models as a top-level API (it's always possible to hack it together using model.set_initial_conditions_from in a for loop).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature refactor refactoring existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants