From 64012268d01bb693aa9cc5c5eba07c23bdd3fe57 Mon Sep 17 00:00:00 2001 From: Valentin Sulzer Date: Wed, 6 Jul 2022 11:57:25 -0400 Subject: [PATCH 1/3] make variables_and_events attribute and remove some events --- pybamm/models/base_model.py | 14 +++++ .../constant_concentration.py | 4 ++ .../submodels/porosity/constant_porosity.py | 4 ++ pybamm/solvers/solution.py | 6 +- tests/unit/test_models/test_base_model.py | 4 ++ tests/unit/test_solvers/test_scipy_solver.py | 58 ++++++++++--------- 6 files changed, 61 insertions(+), 29 deletions(-) diff --git a/pybamm/models/base_model.py b/pybamm/models/base_model.py index feb44a607e..74f2793e3a 100644 --- a/pybamm/models/base_model.py +++ b/pybamm/models/base_model.py @@ -190,6 +190,20 @@ def variables(self, variables): def variable_names(self): return list(self._variables.keys()) + @property + def variables_and_events(self): + """ + Returns variables and events in a single dictionary + """ + try: + return self._variables_and_events + except AttributeError: + self._variables_and_events = self.variables.copy() + self._variables_and_events.update( + {f"Event: {event.name}": event.expression for event in self.events} + ) + return self._variables_and_events + @property def events(self): return self._events diff --git a/pybamm/models/submodels/electrolyte_diffusion/constant_concentration.py b/pybamm/models/submodels/electrolyte_diffusion/constant_concentration.py index f6e0995f0e..09c3ea0c4d 100644 --- a/pybamm/models/submodels/electrolyte_diffusion/constant_concentration.py +++ b/pybamm/models/submodels/electrolyte_diffusion/constant_concentration.py @@ -88,3 +88,7 @@ def set_boundary_conditions(self, variables): "right": (pybamm.Scalar(0), "Neumann"), } } + + def set_events(self, variables): + # No event since the concentration is constant + pass diff --git a/pybamm/models/submodels/porosity/constant_porosity.py b/pybamm/models/submodels/porosity/constant_porosity.py index 383d373635..5e2176d758 100644 --- a/pybamm/models/submodels/porosity/constant_porosity.py +++ b/pybamm/models/submodels/porosity/constant_porosity.py @@ -44,3 +44,7 @@ def get_fundamental_variables(self): ) return variables + + def set_events(self, variables): + # No events since porosity is constant + pass diff --git a/pybamm/solvers/solution.py b/pybamm/solvers/solution.py index 4d816a53f3..81e20c38a9 100644 --- a/pybamm/solvers/solution.py +++ b/pybamm/solvers/solution.py @@ -480,12 +480,14 @@ def update(self, variables): # ProcessedSymbolicVariable if self.has_symbolic_inputs is True: var = pybamm.ProcessedSymbolicVariable( - self.all_models[0].variables[key], self + self.all_models[0].variables_and_events[key], self ) # Otherwise a standard ProcessedVariable is ok else: - vars_pybamm = [model.variables[key] for model in self.all_models] + vars_pybamm = [ + model.variables_and_events[key] for model in self.all_models + ] # Iterate through all models, some may be in the list several times and # therefore only get set up once diff --git a/tests/unit/test_models/test_base_model.py b/tests/unit/test_models/test_base_model.py index 6d246930ac..1cd30f9fff 100644 --- a/tests/unit/test_models/test_base_model.py +++ b/tests/unit/test_models/test_base_model.py @@ -152,6 +152,10 @@ def test_read_parameters(self): v: {"left": (0, "Dirichlet"), "right": (h, "Neumann")}, } + # Test variables_and_events + self.assertIn("v+f+i", model.variables_and_events) + self.assertIn("Event: u=e", model.variables_and_events) + self.assertEqual( set([x.name for x in model.parameters]), set([x.name for x in [a, b, c, d, e, f, g, h, i]]), diff --git a/tests/unit/test_solvers/test_scipy_solver.py b/tests/unit/test_solvers/test_scipy_solver.py index 3a5625aace..510e9c87d2 100644 --- a/tests/unit/test_solvers/test_scipy_solver.py +++ b/tests/unit/test_solvers/test_scipy_solver.py @@ -102,6 +102,9 @@ def test_model_solver_with_event_python(self): np.testing.assert_equal(solution.t_event[0], solution.t[-1]) np.testing.assert_array_equal(solution.y_event[:, 0], solution.y[:, -1]) + # Test event in solution variables_and_events + np.testing.assert_array_almost_equal(solution["Event: var=0.5"].data[-1], 0) + def test_model_solver_ode_with_jacobian_python(self): # Create model model = pybamm.BaseModel() @@ -513,12 +516,11 @@ def test_solve_sensitivity_scalar_var_scalar_input(self): # Solve # Make sure that passing in extra options works - solver = pybamm.ScipySolver( - rtol=1e-10, atol=1e-10 - ) + solver = pybamm.ScipySolver(rtol=1e-10, atol=1e-10) t_eval = np.linspace(0, 1, 80) - solution = solver.solve(model, t_eval, inputs={"p": 0.1}, - calculate_sensitivities=True) + solution = solver.solve( + model, t_eval, inputs={"p": 0.1}, calculate_sensitivities=True + ) np.testing.assert_array_equal(solution.t, t_eval) np.testing.assert_allclose(solution.y[0], np.exp(0.1 * solution.t)) np.testing.assert_allclose( @@ -549,13 +551,13 @@ def test_solve_sensitivity_scalar_var_scalar_input(self): # Solve # Make sure that passing in extra options works - solver = pybamm.ScipySolver( - rtol=1e-10, atol=1e-10 - ) + solver = pybamm.ScipySolver(rtol=1e-10, atol=1e-10) t_eval = np.linspace(0, 1, 80) solution = solver.solve( - model, t_eval, inputs={"p": 0.1, "q": 2, "r": -1, "s": 0.5}, - calculate_sensitivities=True + model, + t_eval, + inputs={"p": 0.1, "q": 2, "r": -1, "s": 0.5}, + calculate_sensitivities=True, ) np.testing.assert_allclose(solution.y[0], -1 + 0.2 * solution.t) np.testing.assert_allclose( @@ -625,8 +627,9 @@ def test_solve_sensitivity_vector_var_scalar_input(self): # Solve - scalar input solver = pybamm.ScipySolver() t_eval = np.linspace(0, 1) - solution = solver.solve(model, t_eval, inputs={"param": 7}, - calculate_sensitivities=True) + solution = solver.solve( + model, t_eval, inputs={"param": 7}, calculate_sensitivities=True + ) np.testing.assert_array_almost_equal( solution["var"].data, np.tile(2 * np.exp(-7 * t_eval), (n, 1)), @@ -657,13 +660,13 @@ def test_solve_sensitivity_vector_var_scalar_input(self): # Solve # Make sure that passing in extra options works - solver = pybamm.ScipySolver( - rtol=1e-10, atol=1e-10 - ) + solver = pybamm.ScipySolver(rtol=1e-10, atol=1e-10) t_eval = np.linspace(0, 1, 80) solution = solver.solve( - model, t_eval, inputs={"p": 0.1, "q": 2, "r": -1, "s": 0.5}, - calculate_sensitivities=True + model, + t_eval, + inputs={"p": 0.1, "q": 2, "r": -1, "s": 0.5}, + calculate_sensitivities=True, ) np.testing.assert_allclose(solution.y, np.tile(-1 + 0.2 * solution.t, (n, 1))) np.testing.assert_allclose( @@ -737,12 +740,14 @@ def test_solve_sensitivity_vector_var_vector_input(self): n = disc.mesh["negative electrode"].npts # Solve - constant input - solver = pybamm.ScipySolver( - rtol=1e-10, atol=1e-10 - ) + solver = pybamm.ScipySolver(rtol=1e-10, atol=1e-10) t_eval = np.linspace(0, 1) - solution = solver.solve(model, t_eval, inputs={"param": 7 * np.ones(n)}, - calculate_sensitivities=True) + solution = solver.solve( + model, + t_eval, + inputs={"param": 7 * np.ones(n)}, + calculate_sensitivities=True, + ) l_n = mesh["negative electrode"].edges[-1] np.testing.assert_array_almost_equal( solution["var"].data, @@ -765,13 +770,12 @@ def test_solve_sensitivity_vector_var_vector_input(self): ) # Solve - linspace input - solver = pybamm.ScipySolver( - rtol=1e-10, atol=1e-10 - ) + solver = pybamm.ScipySolver(rtol=1e-10, atol=1e-10) t_eval = np.linspace(0, 1) p_eval = np.linspace(1, 2, n) - solution = solver.solve(model, t_eval, inputs={"param": p_eval}, - calculate_sensitivities=True) + solution = solver.solve( + model, t_eval, inputs={"param": p_eval}, calculate_sensitivities=True + ) l_n = mesh["negative electrode"].edges[-1] np.testing.assert_array_almost_equal( solution["var"].data, 2 * np.exp(-p_eval[:, np.newaxis] * t_eval), decimal=4 From f1c4abfce9de43b2ab1c4856f3eb49f4a53c1910 Mon Sep 17 00:00:00 2001 From: Valentin Sulzer Date: Thu, 7 Jul 2022 15:31:23 -0400 Subject: [PATCH 2/3] fix test --- tests/unit/test_plotting/test_quick_plot.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_plotting/test_quick_plot.py b/tests/unit/test_plotting/test_quick_plot.py index 5f3217dc0a..ff28a3fc48 100644 --- a/tests/unit/test_plotting/test_quick_plot.py +++ b/tests/unit/test_plotting/test_quick_plot.py @@ -39,6 +39,11 @@ def test_simple_ode_model(self): "c broadcasted positive electrode": pybamm.PrimaryBroadcast( c, "positive particle" ), + "Variable with a very long name": a, + "2D variable": pybamm.FullBroadcast( + 1, "negative particle", {"secondary": "negative electrode"} + ), + "NaN variable": pybamm.Scalar(np.nan), } model._timescale = pybamm.Scalar(1) @@ -116,7 +121,6 @@ def test_simple_ode_model(self): quick_plot.slider_update(0.01) # Test longer name - model.variables["Variable with a very long name"] = model.variables["a"] quick_plot = pybamm.QuickPlot(solution, ["Variable with a very long name"]) quick_plot.plot(0) @@ -187,11 +191,6 @@ def test_simple_ode_model(self): pybamm.QuickPlot(solution, ["a"], spatial_unit="bad unit") # Test 2D variables - model.variables["2D variable"] = disc.process_symbol( - pybamm.FullBroadcast( - 1, "negative particle", {"secondary": "negative electrode"} - ) - ) quick_plot = pybamm.QuickPlot(solution, ["2D variable"]) quick_plot.plot(0) quick_plot.dynamic_plot(testing=True) @@ -256,7 +255,6 @@ def test_simple_ode_model(self): ) # No variable can be NaN - model.variables["NaN variable"] = disc.process_symbol(pybamm.Scalar(np.nan)) with self.assertRaisesRegex( ValueError, "All-NaN variable 'NaN variable' provided" ): From 361cc7905f847ee90fd17b80e6f0c575814e3a41 Mon Sep 17 00:00:00 2001 From: Valentin Sulzer Date: Mon, 11 Jul 2022 10:31:47 -0400 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41a7728a51..bd0c2f325d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,16 @@ # [Unreleased](https://github.com/pybamm-team/PyBaMM/) +## Features + +- Events can now be plotted as a regular variable (under the name "Event: event_name", e.g. "Event: Minimum voltage [V]") ([#2158](https://github.com/pybamm-team/PyBaMM/pull/2158)) + ## Optimizations - Added error for when solution vector gets too large, to help debug solver errors ([#2138](https://github.com/pybamm-team/PyBaMM/pull/2138)) ## Bug fixes +- Some events have been removed in the case where they are constant, i.e. can never be reached ([#2158](https://github.com/pybamm-team/PyBaMM/pull/2158)) - Raise explicit `NotImplementedError` if trying to call `bool()` on a pybamm Symbol (e.g. in an if statement condition) ([#2141](https://github.com/pybamm-team/PyBaMM/pull/2141)) - Fixed bug causing cut-off voltage to change after setting up a simulation with a model ([#2138](https://github.com/pybamm-team/PyBaMM/pull/2138)) - A single solution cycle can now be used as a starting solution for a simulation ([#2138](https://github.com/pybamm-team/PyBaMM/pull/2138))