Merge pull request #301 from pybop-team/300-update-default-casadi-sol…

…ver-mode-to-fast-with-events

Adds `"fast with events"` as default echem solver

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Features
 
 
+- [#301](https://github.com/pybop-team/PyBOP/pull/301) - Updates default echem solver to "fast with events" mode.
 - [#251](https://github.com/pybop-team/PyBOP/pull/251) - Increment PyBaMM > v23.5, remove redundant tests within integration tests, increment citation version, fix examples with incorrect model definitions.
 - [#285](https://github.com/pybop-team/PyBOP/pull/285) - Drop support for Python 3.8.
 - [#275](https://github.com/pybop-team/PyBOP/pull/275) - Adds Maximum a Posteriori (MAP) cost function with corresponding tests.

pybop/models/base_model.py

@@ -359,7 +359,7 @@ def simulate(self, inputs, t_eval) -> np.ndarray[np.float64]:
                         )
                     except Exception as e:
                         print(f"Error: {e}")
-                        return [np.inf]
+                        return {signal: [np.inf] for signal in self.signal}
                 else:
                     return {signal: [np.inf] for signal in self.signal}
 

pybop/models/lithium_ion/base_echem.py

@@ -36,8 +36,12 @@ def __init__(
         self.spatial_methods = (
             spatial_methods or self.pybamm_model.default_spatial_methods
         )
-        self.solver = solver or self.pybamm_model.default_solver
-        self.solver.max_step_decrease_count = 1
+        if solver is None:
+            self.solver = self.pybamm_model.default_solver
+            self.solver.mode = "fast with events"
+            self.solver.max_step_decrease_count = 1
+        else:
+            self.solver = solver
 
         # Internal attributes for the built model are initialized but not set
         self._model_with_set_params = None

tests/integration/test_model_experiment_changes.py

@@ -54,8 +54,8 @@ def test_changing_experiment(self, parameter):
             )
 
         # 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)
+        np.testing.assert_allclose(cost_1, 0, atol=1e-5)
+        np.testing.assert_allclose(cost_2, 0, atol=1e-5)
 
     @pytest.mark.integration
     def test_changing_model(self, parameter):
@@ -81,8 +81,8 @@ def test_changing_model(self, parameter):
             )
 
         # 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)
+        np.testing.assert_allclose(cost_1, 0, atol=1e-5)
+        np.testing.assert_allclose(cost_2, 0, atol=1e-5)
 
     def final_cost(self, solution, model, parameters, init_soc):
         # Compute the cost corresponding to a particular solution

tests/unit/test_models.py

@@ -37,6 +37,18 @@ def test_simulate_without_build_model(self, model):
         ):
             model.simulateS1(None, None)
 
+    @pytest.mark.unit
+    def test_non_default_solver(self):
+        solver = pybamm.CasadiSolver(
+            mode="fast",
+            atol=1e-6,
+            rtol=1e-6,
+        )
+        model = pybop.lithium_ion.SPM(solver=solver)
+        assert model.solver.mode == "fast"
+        assert model.solver.atol == 1e-6
+        assert model.solver.rtol == 1e-6
+
     @pytest.mark.unit
     def test_predict_without_pybamm(self, model):
         model._unprocessed_model = None
@@ -269,5 +281,6 @@ def test_non_converged_solution(self):
         res = problem.evaluate([-0.2, -0.2])
         _, res_grad = problem.evaluateS1([-0.2, -0.2])
 
-        assert np.isinf(res).any()
+        for key in problem.signal:
+            assert np.isinf(res.get(key, [])).any()
         assert np.isinf(res_grad).any()