Fix workflow for changing experiment (#240)

* Update if statement

* Update base_model.py

* Create test_model_experiment_changes.py

* Restrict import

* Rebuild model on problem definition

* Update CHANGELOG.md

* Update t_eval
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

CHANGELOG.md

@@ -19,6 +19,7 @@
 
 ## Bug Fixes
 
+- [#233](https://github.com/pybop-team/PyBOP/pull/233) - Enforces model rebuild on initialisation of a Problem to allow a change of experiment, fixes if statement triggering current function update, updates `predictions` to `simulation` to keep distinction between `predict` and `simulate` and adds `test_changes`.
 - [#123](https://github.com/pybop-team/PyBOP/issues/123) - Reinstates check for availability of parameter sets via PyBaMM upon retrieval by `pybop.ParameterSet.pybamm()`.
 - [#196](https://github.com/pybop-team/PyBOP/issues/196) - Fixes failing observer cost tests.
 - [#63](https://github.com/pybop-team/PyBOP/issues/63) - Removes NLOpt Optimiser from future releases. This is to support deployment to the Apple M-Series platform.

pybop/_experiment.py

@@ -1,7 +1,7 @@
-import pybamm
+from pybamm import Experiment
 
 
-class Experiment(pybamm.Experiment):
+class Experiment(Experiment):
     """
     Wraps the Experiment class for generating experiment conditions for PyBaMM models.
     Credit: PyBaMM

pybop/_problem.py

@@ -200,8 +200,13 @@ def __init__(
             self._model.n_outputs = self.n_outputs
             self._model.n_time_data = self.n_time_data
 
-        # Build the model
-        if self._model._built_model is None:
+            # Build the model from scratch
+            if self._model._built_model is not None:
+                self._model._model_with_set_params = None
+                self._model._built_model = None
+                self._model._built_initial_soc = None
+                self._model._mesh = None
+                self._model._disc = None
             self._model.build(
                 dataset=self._dataset,
                 parameters=self.parameters,

pybop/models/base_model.py

@@ -75,7 +75,7 @@ def build(
         Construct the PyBaMM model if not already built, and set parameters.
 
         This method initializes the model components, applies the given parameters,
-        sets up the mesh and discretization if needed, and prepares the model
+        sets up the mesh and discretisation if needed, and prepares the model
         for simulations.
 
         Parameters
@@ -92,7 +92,7 @@ def build(
         self.dataset = dataset
         self.parameters = parameters
         if self.parameters is not None:
-            self.set_parameter_classification(self.parameters)
+            self.classify_and_update_parameters(self.parameters)
             self.fit_keys = [param.name for param in self.parameters]
         else:
             self.fit_keys = []
@@ -106,6 +106,7 @@ def build(
         elif self.pybamm_model.is_discretised:
             self._model_with_set_params = self.pybamm_model
             self._built_model = self.pybamm_model
+
         else:
             self.set_params()
 
@@ -156,11 +157,10 @@ def set_params(self, rebuild=False):
             return
 
         # Mark any simulation inputs in the parameter set
-        if self.non_matched_parameters:
-            for i in self.fit_keys:
-                self._parameter_set[i] = "[input]"
+        for key in self.non_matched_parameters.keys():
+            self._parameter_set[key] = "[input]"
 
-        if self.dataset is not None and self.non_matched_parameters:
+        if self.dataset is not None and (not self.matched_parameters or not rebuild):
             if "Current function [A]" not in self.fit_keys:
                 self._parameter_set["Current function [A]"] = pybamm.Interpolant(
                     self.dataset["Time [s]"],
@@ -190,8 +190,8 @@ def rebuild(
 
         This method requires the self.build() method to be called first, and
         then rebuilds the model for a given parameter set. Specifically,
-        this method applies the given parameters, sets up the mesh and discretization if needed, and prepares the model
-        for simulations.
+        this method applies the given parameters, sets up the mesh and
+        discretisation if needed, and prepares the model for simulations.
 
         Parameters
         ----------
@@ -209,7 +209,7 @@ def rebuild(
         self.dataset = dataset
         self.parameters = parameters
         if parameters is not None:
-            self.set_parameter_classification(parameters)
+            self.classify_and_update_parameters(parameters)
 
         if init_soc is not None:
             self.set_init_soc(init_soc)
@@ -227,29 +227,26 @@ def rebuild(
         # Clear solver and setup model
         self._solver._model_set_up = {}
 
-    def set_parameter_classification(self, parameters):
+    def classify_and_update_parameters(self, parameters):
         """
-        Set the parameter classification for the model.
+        Update the parameter values according to their classification as either
+        'matched_parameters' which require a model rebuild and
+        'non_matched_parameters' which are standard inputs.
 
         Parameters
         ----------
-        parameters : Pybop.ParameterSet
-
-        Returns
-        -------
-        None
-            The method updates attributes on self.
+        parameters : pybop.ParameterSet
 
         """
-        processed_parameters = {param.name: param.value for param in parameters}
+        parameter_dictionary = {param.name: param.value for param in parameters}
         matched_parameters = {
-            param: processed_parameters[param]
-            for param in processed_parameters
+            param: parameter_dictionary[param]
+            for param in parameter_dictionary
             if param in self.rebuild_parameters
         }
         non_matched_parameters = {
-            param: processed_parameters[param]
-            for param in processed_parameters
+            param: parameter_dictionary[param]
+            for param in parameter_dictionary
             if param not in self.rebuild_parameters
         }
 
@@ -261,9 +258,6 @@ def set_parameter_classification(self, parameters):
             self._unprocessed_parameter_set = self._parameter_set
             self.geometry = self.pybamm_model.default_geometry
 
-        if self.non_matched_parameters:
-            self.fit_keys = list(self.non_matched_parameters.keys())
-
     def reinit(
         self, inputs: Inputs, t: float = 0.0, x: Optional[np.ndarray] = None
     ) -> TimeSeriesState:
@@ -305,7 +299,7 @@ def step(self, state: TimeSeriesState, time: np.ndarray) -> TimeSeriesState:
         state : TimeSeriesState
             The current state of the model
         time : np.ndarray
-            The time to predict the system to (in whatever time units the model is in)
+            The time to simulate the system until (in whatever time units the model is in)
         """
         dt = time - state.t
         new_sol = self._solver.step(
@@ -338,7 +332,7 @@ def simulate(self, inputs, t_eval) -> np.ndarray[np.float64]:
         if self._built_model is None:
             raise ValueError("Model must be built before calling simulate")
         else:
-            if not self.fit_keys and self.matched_parameters:
+            if self.matched_parameters and not self.non_matched_parameters:
                 sol = self.solver.solve(self.built_model, t_eval=t_eval)
 
             else:
@@ -355,9 +349,9 @@ def simulate(self, inputs, t_eval) -> np.ndarray[np.float64]:
                 else:
                     return [np.inf]
 
-            predictions = [sol[signal].data for signal in self.signal]
+            simulation = [sol[signal].data for signal in self.signal]
 
-            return np.vstack(predictions).T
+            return np.vstack(simulation).T
 
     def simulateS1(self, inputs, t_eval):
         """
@@ -386,6 +380,11 @@ def simulateS1(self, inputs, t_eval):
         if self._built_model is None:
             raise ValueError("Model must be built before calling simulate")
         else:
+            if self.matched_parameters:
+                raise ValueError(
+                    "Cannot use sensitivies for parameters which require a model rebuild"
+                )
+
             if not isinstance(inputs, dict):
                 inputs = {key: inputs[i] for i, key in enumerate(self.fit_keys)}
 
@@ -400,7 +399,7 @@ def simulateS1(self, inputs, t_eval):
                     calculate_sensitivities=True,
                 )
 
-                predictions = [sol[signal].data for signal in self.signal]
+                simulation = [sol[signal].data for signal in self.signal]
 
                 sensitivities = [
                     np.array(
@@ -409,7 +408,7 @@ def simulateS1(self, inputs, t_eval):
                     for key in self.fit_keys
                 ]
 
-                return np.vstack(predictions).T, np.dstack(sensitivities)
+                return np.vstack(simulation).T, np.dstack(sensitivities)
 
             else:
                 return [np.inf], [np.inf]
@@ -460,7 +459,7 @@ def predict(
             if PyBaMM models are not supported by the current simulation method.
 
         """
-        parameter_set = parameter_set or self._parameter_set
+        parameter_set = parameter_set or self._unprocessed_parameter_set
         if inputs is not None:
             if not isinstance(inputs, dict):
                 inputs = {key: inputs[i] for i, key in enumerate(self.fit_keys)}

tests/integration/test_model_experiment_changes.py

@@ -0,0 +1,103 @@
+import pybop
+import pytest
+import numpy as np
+
+
+class TestModelAndExperimentChanges:
+    """
+    A class to test that different inputs return different outputs.
+    """
+
+    @pytest.fixture(
+        params=[
+            pybop.Parameter(  # geometric parameter
+                "Negative particle radius [m]",
+                prior=pybop.Gaussian(6e-06, 0.1e-6),
+                bounds=[1e-6, 9e-6],
+                true_value=5.86e-6,
+            ),
+            pybop.Parameter(  # non-geometric parameter
+                "Positive electrode diffusivity [m2.s-1]",
+                prior=pybop.Gaussian(3.43e-15, 1e-15),
+                bounds=[1e-15, 5e-15],
+                true_value=4e-15,
+            ),
+        ]
+    )
+    def parameter(self, request):
+        return request.param
+
+    @pytest.mark.integration
+    def test_changing_experiment(self, parameter):
+        # Change the experiment and check that the results are different.
+
+        parameter_set = pybop.ParameterSet.pybamm("Chen2020")
+        parameters = [parameter]
+        init_soc = 0.5
+        model = pybop.lithium_ion.SPM(parameter_set=parameter_set)
+
+        t_eval = np.arange(0, 3600, 2)  # Default 1C discharge to cut-off voltage
+        solution_1 = model.predict(init_soc=init_soc, t_eval=t_eval)
+        cost_1 = self.final_cost(solution_1, model, parameters, init_soc)
+
+        experiment = pybop.Experiment(["Charge at 1C until 4.1 V (2 seconds period)"])
+        solution_2 = model.predict(
+            init_soc=init_soc, experiment=experiment, inputs=[parameter.true_value]
+        )
+        cost_2 = self.final_cost(solution_2, model, parameters, init_soc)
+
+        with np.testing.assert_raises(AssertionError):
+            np.testing.assert_array_equal(
+                solution_1["Voltage [V]"].data,
+                solution_2["Voltage [V]"].data,
+            )
+
+        # The datasets are not corrupted so the costs should be zero
+        np.testing.assert_almost_equal(cost_1, 0)
+        np.testing.assert_almost_equal(cost_2, 0)
+
+    @pytest.mark.integration
+    def test_changing_model(self, parameter):
+        # Change the model and check that the results are different.
+
+        parameter_set = pybop.ParameterSet.pybamm("Chen2020")
+        parameters = [parameter]
+        init_soc = 0.5
+        experiment = pybop.Experiment(["Charge at 1C until 4.1 V (2 seconds period)"])
+
+        model = pybop.lithium_ion.SPM(parameter_set=parameter_set)
+        solution_1 = model.predict(init_soc=init_soc, experiment=experiment)
+        cost_1 = self.final_cost(solution_1, model, parameters, init_soc)
+
+        model = pybop.lithium_ion.SPMe(parameter_set=parameter_set)
+        solution_2 = model.predict(init_soc=init_soc, experiment=experiment)
+        cost_2 = self.final_cost(solution_2, model, parameters, init_soc)
+
+        with np.testing.assert_raises(AssertionError):
+            np.testing.assert_array_equal(
+                solution_1["Voltage [V]"].data,
+                solution_2["Voltage [V]"].data,
+            )
+
+        # The datasets are not corrupted so the costs should be zero
+        np.testing.assert_almost_equal(cost_1, 0)
+        np.testing.assert_almost_equal(cost_2, 0)
+
+    def final_cost(self, solution, model, parameters, init_soc):
+        # Compute the cost corresponding to a particular solution
+        x0 = np.array([parameters[0].true_value])
+        dataset = pybop.Dataset(
+            {
+                "Time [s]": solution["Time [s]"].data,
+                "Current function [A]": solution["Current [A]"].data,
+                "Voltage [V]": solution["Voltage [V]"].data,
+            }
+        )
+        signal = ["Voltage [V]"]
+        problem = pybop.FittingProblem(
+            model, parameters, dataset, signal=signal, x0=x0, init_soc=init_soc
+        )
+        cost = pybop.RootMeanSquaredError(problem)
+        optim = pybop.Optimisation(cost, optimiser=pybop.PSO)
+        x, final_cost = optim.run()
+        return final_cost