Merge pull request #465 from pybop-team/464-design-updating

Refactor update_capacity

CHANGELOG.md

@@ -23,6 +23,7 @@
 
 ## Bug Fixes
 
+- [#464](https://github.com/pybop-team/PyBOP/issues/464) - Fix order of design `parameter_set` updates and refactor `update_capacity`.
 - [#468](https://github.com/pybop-team/PyBOP/issue/468) - Renames `quick_plot.py` to `standard_plots.py`.
 - [#454](https://github.com/pybop-team/PyBOP/issue/454) - Fixes benchmarking suite.
 - [#421](https://github.com/pybop-team/PyBOP/issues/421) - Adds a default value for the initial SOC for design problems.

examples/scripts/spme_max_energy.py

@@ -35,13 +35,18 @@
 
 # Generate problem
 problem = pybop.DesignProblem(
-    model, parameters, experiment, signal=signal, initial_state={"Initial SoC": 1.0}
+    model,
+    parameters,
+    experiment,
+    signal=signal,
+    initial_state={"Initial SoC": 1.0},
+    update_capacity=True,
 )
 
-# Generate multiple cost functions and combine them.
-cost1 = pybop.GravimetricEnergyDensity(problem, update_capacity=True)
-cost2 = pybop.VolumetricEnergyDensity(problem, update_capacity=True)
-cost = pybop.WeightedCost(cost1, cost2, weights=[1, 1])
+# Generate multiple cost functions and combine them
+cost1 = pybop.GravimetricEnergyDensity(problem)
+cost2 = pybop.VolumetricEnergyDensity(problem)
+cost = pybop.WeightedCost(cost1, cost2, weights=[1, 1e-3])
 
 # Run optimisation
 optim = pybop.PSO(
@@ -55,10 +60,7 @@
 print(f"Optimised volumetric energy density: {cost2(x):.2f} Wh.m-3")
 
 # Plot the timeseries output
-if cost.update_capacity:
-    problem.model.approximate_capacity(x)
 pybop.quick_plot(problem, problem_inputs=x, title="Optimised Comparison")
 
 # Plot the cost landscape with optimisation path
-if len(x) == 2:
-    pybop.plot2d(optim, steps=3)
+pybop.plot2d(optim, steps=5)

examples/standalone/problem.py

@@ -40,7 +40,7 @@ def __init__(
                 )
         self._target = {signal: self._dataset[signal] for signal in self.signal}
 
-    def evaluate(self, inputs, **kwargs):
+    def evaluate(self, inputs):
         """
         Evaluate the model with the given parameters and return the signal.
 

pybop/costs/_weighted_cost.py

@@ -1,4 +1,3 @@
-import warnings
 from typing import Optional, Union
 
 import numpy as np
@@ -64,18 +63,6 @@ def __init__(self, *costs, weights: Optional[list[float]] = None):
             for cost in self.costs:
                 self.parameters.join(cost.parameters)
 
-        # Check if any cost function requires capacity update
-        self.update_capacity = False
-        if any(cost.update_capacity for cost in self.costs):
-            self.update_capacity = True
-
-            warnings.warn(
-                "WeightedCost doesn't currently support DesignCosts with different `update_capacity` attributes,\n"
-                f"Using global `DesignCost.update_capacity` attribute as: {self.update_capacity}",
-                UserWarning,
-                stacklevel=2,
-            )
-
         # Weighted costs do not use this functionality
         self._has_separable_problem = False
 
@@ -107,7 +94,7 @@ def compute(
             if calculate_grad:
                 y, dy = self.problem.evaluateS1(inputs)
             else:
-                y = self.problem.evaluate(inputs, update_capacity=self.update_capacity)
+                y = self.problem.evaluate(inputs)
 
         e = np.empty_like(self.costs)
         de = np.empty((len(self.parameters), len(self.costs)))
@@ -120,9 +107,7 @@ def compute(
                 if calculate_grad:
                     y, dy = cost.problem.evaluateS1(inputs)
                 else:
-                    y = cost.problem.evaluate(
-                        inputs, update_capacity=self.update_capacity
-                    )
+                    y = cost.problem.evaluate(inputs)
 
             if calculate_grad:
                 e[i], de[:, i] = cost.compute(y, dy=dy, calculate_grad=True)

pybop/costs/base_cost.py

@@ -36,7 +36,6 @@ def __init__(self, problem: Optional[BaseProblem] = None):
         self.problem = problem
         self.verbose = False
         self._has_separable_problem = False
-        self.update_capacity = False
         self.y = None
         self.dy = None
         self._de = 1.0
@@ -93,10 +92,7 @@ def __call__(self, inputs: Union[Inputs, list], calculate_grad: bool = False):
                 if calculate_grad is True:
                     y, dy = self.problem.evaluateS1(self.problem.parameters.as_dict())
                 else:
-                    y = self.problem.evaluate(
-                        self.problem.parameters.as_dict(),
-                        update_capacity=self.update_capacity,
-                    )
+                    y = self.problem.evaluate(self.problem.parameters.as_dict())
 
             return self.compute(y, dy=dy, calculate_grad=calculate_grad)
 

pybop/costs/design_costs.py

@@ -1,9 +1,6 @@
-import warnings
-
 import numpy as np
 
 from pybop.costs.base_cost import BaseCost
-from pybop.parameters.parameter import Inputs
 
 
 class DesignCost(BaseCost):
@@ -16,13 +13,9 @@ class DesignCost(BaseCost):
     ---------------------
     problem : object
         The associated problem containing model and evaluation methods.
-    parameter_set : object)
-        The set of parameters from the problem's model.
-    dt : float
-        The time step size used in the simulation.
     """
 
-    def __init__(self, problem, update_capacity=False):
+    def __init__(self, problem):
         """
         Initialises the gravimetric energy density calculator with a problem.
 
@@ -33,37 +26,6 @@ def __init__(self, problem, update_capacity=False):
         """
         super().__init__(problem)
         self.problem = problem
-        if update_capacity is True:
-            nominal_capacity_warning = (
-                "The nominal capacity is approximated for each iteration."
-            )
-        else:
-            nominal_capacity_warning = (
-                "The nominal capacity is fixed at the initial model value."
-            )
-        warnings.warn(nominal_capacity_warning, UserWarning, stacklevel=2)
-        self.update_capacity = update_capacity
-        self.parameter_set = problem.model.parameter_set
-        self.update_simulation_data(self.parameters.as_dict("initial"))
-
-    def update_simulation_data(self, inputs: Inputs):
-        """
-        Updates the simulation data based on the initial parameter values.
-
-        Parameters
-        ----------
-        inputs : Inputs
-            The initial parameter values for the simulation.
-        """
-        if self.update_capacity:
-            self.problem.model.approximate_capacity(inputs)
-        solution = self.problem.evaluate(inputs)
-
-        if "Time [s]" not in solution:
-            raise ValueError("The solution does not contain time data.")
-        self.problem.domain_data = solution["Time [s]"]
-        self.problem.target = {key: solution[key] for key in self.problem.signal}
-        self.dt = solution["Time [s]"][1] - solution["Time [s]"][0]
 
 
 class GravimetricEnergyDensity(DesignCost):
@@ -76,9 +38,8 @@ class GravimetricEnergyDensity(DesignCost):
     Inherits all parameters and attributes from ``DesignCost``.
     """
 
-    def __init__(self, problem, update_capacity=False):
-        super().__init__(problem, update_capacity)
-        self._fixed_problem = False  # keep problem evaluation within compute
+    def __init__(self, problem):
+        super().__init__(problem)
 
     def compute(
         self,
@@ -108,8 +69,9 @@ def compute(
             return -np.inf
 
         voltage, current = y["Voltage [V]"], y["Current [A]"]
-        energy_density = np.trapz(voltage * current, dx=self.dt) / (
-            3600 * self.problem.model.cell_mass(self.parameter_set)
+        dt = y["Time [s]"][1] - y["Time [s]"][0]
+        energy_density = np.trapz(voltage * current, dx=dt) / (
+            3600 * self.problem.model.cell_mass()
         )
 
         return energy_density
@@ -125,8 +87,8 @@ class VolumetricEnergyDensity(DesignCost):
     Inherits all parameters and attributes from ``DesignCost``.
     """
 
-    def __init__(self, problem, update_capacity=False):
-        super().__init__(problem, update_capacity)
+    def __init__(self, problem):
+        super().__init__(problem)
 
     def compute(
         self,
@@ -156,8 +118,9 @@ def compute(
             return -np.inf
 
         voltage, current = y["Voltage [V]"], y["Current [A]"]
-        energy_density = np.trapz(voltage * current, dx=self.dt) / (
-            3600 * self.problem.model.cell_volume(self.parameter_set)
+        dt = y["Time [s]"][1] - y["Time [s]"][0]
+        energy_density = np.trapz(voltage * current, dx=dt) / (
+            3600 * self.problem.model.cell_volume()
         )
 
         return energy_density

pybop/models/base_model.py

@@ -81,9 +81,8 @@ def __init__(
         ----------
         name : str, optional
             The name given to the model instance.
-        parameter_set : pybop.ParameterSet, optional
-            A PyBOP ParameterSet, PyBaMM ParameterValues object or a dictionary containing the
-            parameter values.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
         check_params : Callable, optional
             A compatibility check for the model parameters. Function, with
             signature
@@ -695,9 +694,9 @@ def predict(
         t_eval : array-like, optional
             An array of time points at which to evaluate the solution. Defaults to None,
             which means the time points need to be specified within experiment or elsewhere.
-        parameter_set : pybamm.ParameterValues, optional
-            A PyBaMM ParameterValues object or a dictionary containing the parameter values
-            to use for the simulation. Defaults to the model's current ParameterValues if None.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values to use for the simulation.
+            Defaults to the model's current ParameterValues if None.
         experiment : pybamm.Experiment, optional
             A PyBaMM Experiment object specifying the experimental conditions under which
             the simulation should be run. Defaults to None, indicating no experiment.
@@ -726,14 +725,16 @@ def predict(
         elif not self._unprocessed_model._built:  # noqa: SLF001
             self._unprocessed_model.build_model()
 
+        no_parameter_set = parameter_set is None
         parameter_set = parameter_set or self._unprocessed_parameter_set.copy()
         if inputs is not None:
             inputs = self.parameters.verify(inputs)
             parameter_set.update(inputs)
 
         if initial_state is not None:
-            # Update the default initial state for consistency
-            self.set_initial_state(initial_state)
+            if no_parameter_set:
+                # Update the default initial state for consistency
+                self.set_initial_state(initial_state)
 
             initial_state = self.convert_to_pybamm_initial_state(initial_state)
             if isinstance(self.pybamm_model, pybamm.equivalent_circuit.Thevenin):
@@ -776,8 +777,8 @@ def check_params(
         ----------
         inputs : Inputs
             The input parameters for the simulation.
-        parameter_set : pybop.ParameterSet, optional
-            A PyBOP parameter set object or a dictionary containing the parameter values.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
         allow_infeasible_solutions : bool, optional
             If True, infeasible parameter values will be allowed in the optimisation (default: True).
 
@@ -810,8 +811,8 @@ def _check_params(
         ----------
         inputs : Inputs
             The input parameters for the simulation.
-        parameter_set : pybop.ParameterSet
-            A PyBOP parameter set object or a dictionary containing the parameter values.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
         allow_infeasible_solutions : bool, optional
             If True, infeasible parameter values will be allowed in the optimisation (default: True).
 
@@ -890,9 +891,8 @@ def cell_mass(self, parameter_set: ParameterSet = None):
 
         Parameters
         ----------
-        parameter_set : dict, optional
-            A dictionary containing the parameter values necessary for the mass
-            calculations.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
 
         Raises
         ------
@@ -909,9 +909,8 @@ def cell_volume(self, parameter_set: ParameterSet = None):
 
         Parameters
         ----------
-        parameter_set : dict, optional
-            A dictionary containing the parameter values necessary for the volume
-            calculation.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
 
         Raises
         ------
@@ -920,17 +919,17 @@ def cell_volume(self, parameter_set: ParameterSet = None):
         """
         raise NotImplementedError
 
-    def approximate_capacity(self, inputs: Inputs):
+    def approximate_capacity(self, parameter_set: ParameterSet = None):
         """
-        Calculate a new estimate for the nominal capacity based on the theoretical energy density
-        and an average voltage.
+        Calculate a new estimate for the nominal capacity based on the theoretical energy
+        density and an average voltage.
 
         This method must be implemented by subclasses.
 
         Parameters
         ----------
-        inputs : Inputs
-            The parameters that are the inputs of the model.
+        parameter_set : Union[pybop.ParameterSet, pybamm.ParameterValues], optional
+            A dict-like object containing the parameter values.
 
         Raises
         ------