Repository: pybamm-team/PyBaMM (python)

Issue:
Make `calc_esoh` a model attribute
Currently the `Simulation` class uses the following logic to decide if it should calculate the state of health in a simulation 

```
    def _get_esoh_solver(self, calc_esoh):
        if (
            calc_esoh is False
            or isinstance(self._model, pybamm.lead_acid.BaseModel)
            or isinstance(self._model, pybamm.equivalent_circuit.Thevenin)
            or self._model.options["working electrode"] != "both"
        ):
            return None

        return pybamm.lithium_ion.ElectrodeSOHSolver(
            self._parameter_values, self._model.param, options=self._model.options
        )
```

It would be nice if `cacl_esoh` was a model attribute (`True` by default) so that when writing custom models you don't have to pass `calc_esoh=False` to the solver every time.

Pull request:
Creates a 'calc_esoh' property in the BaseModel
Creates a 'cal_esoh' property in the model class, set to False for the BaseModel, and True for the lithium ion BaseModel.
Allows the logic for whether eSOH variables should be calculated to be stored in the model, while allowing `Simulation.solve()` to override the model setting (i.e. set calc_esoh to False) as is the current behaviour.

Fixes #4619 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ac422425c..8d8712134 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Features
 
+- Creates a 'calc_esoh' property in battery models ([#4825](https://github.com/pybamm-team/PyBaMM/pull/4825))
 - Added 'get_summary_variables' to return dictionary of computed summary variables ([#4824](https://github.com/pybamm-team/PyBaMM/pull/4824))
 - Added support for particle size distributions combined with particle mechanics. ([#4807](https://github.com/pybamm-team/PyBaMM/pull/4807))
 
diff --git a/src/pybamm/models/base_model.py b/src/pybamm/models/base_model.py
index 3c8fa9148..a4e58600b 100644
--- a/src/pybamm/models/base_model.py
+++ b/src/pybamm/models/base_model.py
@@ -80,6 +80,9 @@ class BaseModel:
         self.is_discretised = False
         self.y_slices = None
 
+        # Non-lithium ion models shouldn't calculate eSOH parameters
+        self._calc_esoh = False
+
     @classmethod
     def deserialise(cls, properties: dict):
         """
@@ -426,6 +429,11 @@ class BaseModel:
             self._input_parameters = self._find_symbols(pybamm.InputParameter)
         return self._input_parameters
 
+    @property
+    def calc_esoh(self):
+        """Whether to include eSOH variables in the summary variables."""
+        return self._calc_esoh
+
     def get_parameter_info(self, by_submodel=False):
         """
         Extracts the parameter information and returns it as a dictionary.
diff --git a/src/pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py b/src/pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py
index 9b801e113..3c9595eb2 100644
--- a/src/pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py
+++ b/src/pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py
@@ -33,6 +33,9 @@ class BaseModel(pybamm.BaseBatteryModel):
 
         self.set_standard_output_variables()
 
+        # Li models should calculate esoh by default
+        self._calc_esoh = True
+
     def set_submodels(self, build):
         self.set_external_circuit_submodel()
         self.set_porosity_submodel()
@@ -93,6 +96,24 @@ class BaseModel(pybamm.BaseBatteryModel):
                 "Voltage [V]",
             ]
 
+    @property
+    def calc_esoh(self):
+        """Whether to include eSOH variables in the summary variables."""
+        if (
+            self.options["particle phases"] not in ["1", ("1", "1")]
+            or self.options["working electrode"] != "both"
+        ):
+            self._calc_esoh = False
+        return self._calc_esoh
+
+    @calc_esoh.setter
+    def calc_esoh(self, value: bool):
+        """Set whether to include eSOH variables in the summary variables."""
+        if not isinstance(value, bool):
+            raise TypeError("`calc_esoh` arg needs to be a bool")
+
+        self._calc_esoh = value
+
     def set_standard_output_variables(self):
         super().set_standard_output_variables()
 
diff --git a/src/pybamm/simulation.py b/src/pybamm/simulation.py
index cf84755e7..448b750d0 100644
--- a/src/pybamm/simulation.py
+++ b/src/pybamm/simulation.py
@@ -393,7 +393,7 @@ class Simulation:
         t_eval=None,
         solver=None,
         save_at_cycles=None,
-        calc_esoh=True,
+        calc_esoh=None,
         starting_solution=None,
         initial_soc=None,
         callbacks=None,
@@ -436,7 +436,8 @@ class Simulation:
         calc_esoh : bool, optional
             Whether to include eSOH variables in the summary variables. If `False`
             then only summary variables that do not require the eSOH calculation
-            are calculated. Default is True.
+            are calculated.
+            If given, overwrites the default provided by the model.
         starting_solution : :class:`pybamm.Solution`
             The solution to start stepping from. If None (default), then self._solution
             is used. Must be None if not using an experiment.
@@ -464,6 +465,20 @@ class Simulation:
         if solver is None:
             solver = self._solver
 
+        if calc_esoh is None:
+            calc_esoh = self._model.calc_esoh
+        else:
+            # stop 'True' override if model isn't suitable to calculate eSOH
+            if calc_esoh and not self._model.calc_esoh:
+                calc_esoh = False
+                warnings.warn(
+                    UserWarning(
+                        "Model is not suitable for calculating eSOH, "
+                        "setting `calc_esoh` to False",
+                    ),
+                    stacklevel=2,
+                )
+
         callbacks = pybamm.callbacks.setup_callbacks(callbacks)
         logs = {}
 
@@ -1021,12 +1036,7 @@ class Simulation:
         return self._solution
 
     def _get_esoh_solver(self, calc_esoh):
-        if (
-            calc_esoh is False
-            or not isinstance(self._model, pybamm.lithium_ion.BaseModel)
-            or self._model.options["particle phases"] not in ["1", ("1", "1")]
-            or self._model.options["working electrode"] != "both"
-        ):
+        if calc_esoh is False:
             return None
 
         return pybamm.lithium_ion.ElectrodeSOHSolver(


Test patch:
diff --git a/tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_base_lithium_ion_model.py b/tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_base_lithium_ion_model.py
index bfeb48966..b85ce7e00 100644
--- a/tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_base_lithium_ion_model.py
+++ b/tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_base_lithium_ion_model.py
@@ -42,3 +42,11 @@ class TestBaseLithiumIonModel:
             NotImplementedError, match="Reference electrode can only be"
         ):
             model.insert_reference_electrode()
+
+    def test_setting_calc_esoh(self):
+        model = pybamm.lithium_ion.BaseModel()
+        model.calc_esoh = False
+        assert model.calc_esoh is False
+
+        with pytest.raises(TypeError, match="`calc_esoh` arg needs to be a bool"):
+            model.calc_esoh = "Yes"
diff --git a/tests/unit/test_simulation.py b/tests/unit/test_simulation.py
index ded128f49..c88959c1f 100644
--- a/tests/unit/test_simulation.py
+++ b/tests/unit/test_simulation.py
@@ -678,3 +678,15 @@ class TestSimulation:
         inputs = {"Electrode height [m]": 0.2}
         sim = pybamm.Simulation(model=model, parameter_values=parameter_values)
         sim.solve(t_eval=t_eval, inputs=inputs)
+
+    def test_simulation_cannot_force_calc_esoh(self):
+        model = pybamm.BaseModel()
+        v = pybamm.Variable("v")
+        model.rhs = {v: -v}
+        model.initial_conditions = {v: 1}
+        sim = pybamm.Simulation(model)
+
+        with pytest.warns(
+            UserWarning, match="Model is not suitable for calculating eSOH"
+        ):
+            sim.solve([0, 1], calc_esoh=True)