model.new_copy() does not keep modified submodels

[brosaplanella]

Describe the bug
When a standard model is defined (e.g. SPMe) but one of the submodels changed then model.new_copy() creates a copy of the standard model as it runs new_model = self.__class__(options). An example of this is when trying to compare two different submodels with an experiment.

Not sure how easy it would be to fix it through model.new_copy(). In any case, I think it would be interesting to be able to change any submodel through model options. To avoid people needing to change how the model is defined, we could extend the model class so you can pass the option {"electrolyte": pybamm.MyNewElectrolyteSubmodel()}. Later on, if the model consolidates, we can name it and add it as a standard option {"electrolyte": "my new electrolyte submodel"}.

To Reproduce
Steps to reproduce the behaviour:

1. Create two instances of a model (e.g. SPMe) and change a submodel for one of them.
2. Run a simulation with experiments
3. Plot solutions
4. See error

[valentinsulzer]

We should definitely fix new_copy, which shouldn't be too hard (at least it is just one function to change).
Not 100% convinced about passing a model to options as

1. It requires either an extra if statement in all of the model methods (like set_electrolyte_submodel), or just doing it on a case-by-case basis
2. It allows two ways of doing the same thing, which is generally not recommended

[brosaplanella]

Then, would an extra line saying

new_model.submodels = self.submodels

fix it or is it more complex than that?

[valentinsulzer]

Urgh, actually it is a little bit tricky, because you also have to respect this line for doing a simulation with an experiment:

self.model = model.new_copy(
            options={
                **model.options,
                "operating mode": constant_current_constant_voltage_constant_power,
            }
        )

Maybe the thing to do is remove the options argument for model.new_copy and instead do something like

def new_copy(self, build=False):
        "Create a new copy of the model"
        new_model = self.__class__(build=True)
        new_model.submodels = self.submodels
        new_model.name = self.name
        new_model.use_jacobian = self.use_jacobian
        new_model.use_simplify = self.use_simplify
        new_model.convert_to_format = self.convert_to_format
        new_model.timescale = self.timescale
        if build:
            new_model.build_model()

and then for the experiment

new_model = model.new_copy(
            build=False
        )
new_model["external circuit"] = pybamm.external_circuit.FunctionControl(
                new_model.param, constant_current_constant_voltage_constant_power
            )
new_model.build_model()
self.model = new_model

[valentinsulzer]

It probably makes sense for BaseBatteryModel to overwrite the new_copy function with the above changes, since BaseModel doesn't take in a "build" argument nor have "submodels"

[valentinsulzer]

This is harder than planned. I can't get it to work cleanly while retaining the option of updating model options in the simulation. Maybe we should get rid of that functionality, and say that you can't update options (you have to pass in a new model).

[brosaplanella]

I see. Yes, we can get rid of that functionality and pass a new model for now, but in the long run I think it would be good to have an easy way to change one of the submodels without the need of creating a new model file (which can be quite annoying if we just want to play around with submodels).

From the comments above, I understand that the problem is the distinction between the options that change some of the submodels and the operating mode. In this case, would it be better for the options to be {"model options": {dict of model options}, "operating mode": constant_current_constant_voltage_constant_power}?

[valentinsulzer]

The problem is that you can either tell it to update the options, or copy the submodels (and possibly replace one of them), but not both. For example, how would you handle the case where you want to keep a custom electrolyte conductivity model, but update the thermal option.

[brosaplanella]

But wouldn't then be a matter of updating the "model options" with a new dictionary (with the custom electrolyte conductivity and your updated thermal model) and building the model again?

Anyway, I am happy with passing a new model. Is there a way that we can create it in the same script as we are running or do we always need a new file where we define the class?

[valentinsulzer]

But you would have to specify that the electrolyte conductivity had changed, so the default would remove that change. Anyway, it makes sense for a "copy" function to just copy and not change anything while doing so.

[rtimms]

I don't know if this sensible to address at the same time, but if you do something like the below

model = pybamm.lithium_ion.SPM()
param = model.default_parameter_values

sim = pybamm.Simulation(model, parameter_values=param)
sim.solve()

param["Current function [A]"] = 2
sim2 = pybamm.Simulation(model, parameter_values=param)
sim2.solve()

and then compare, you find that the current plotted in both sims is the same (but the other outputs have changed appropriately). Instead you have to pass a new model to sim2. This isn't a huge deal, but users might think they can make multiple simulations using the same model object.

[rtimms]

I see. Yes, we can get rid of that functionality and pass a new model for now, but in the long run I think it would be good to have an easy way to change one of the submodels without the need of creating a new model file (which can be quite annoying if we just want to play around with submodels).

I think with what Tino is suggesting you can still easily change a submodel without creating a new model file. You just cant update the options via the simulation class, right?