From feab7aa6ce79007b900daab2bb8217614d408ca4 Mon Sep 17 00:00:00 2001 From: John Brittain Date: Sat, 3 Feb 2024 00:07:58 +0000 Subject: [PATCH 1/4] Attempting to access variables that are not in the solution now fails gracefully --- pybamm/solvers/solution.py | 20 ++++++- tests/unit/test_solvers/test_idaklu_solver.py | 55 ++++++++++++++----- 2 files changed, 59 insertions(+), 16 deletions(-) diff --git a/pybamm/solvers/solution.py b/pybamm/solvers/solution.py index d481768713..43aba4b955 100644 --- a/pybamm/solvers/solution.py +++ b/pybamm/solvers/solution.py @@ -457,6 +457,16 @@ def set_summary_variables(self, all_summary_variables): {name: np.array(value) for name, value in summary_variables.items()} ) + def _exprtree_contains(self, expr, expr_type): + """Does the expression tree contain a given type of node?""" + if type(expr) is expr_type: + return True + elif "children" in dir(expr): + for child in expr.children: + if self._exprtree_contains(child, expr_type): + return True + return False + def update(self, variables): """Add ProcessedVariables to the dictionary of variables in the solution""" # make sure that sensitivities are extracted if required @@ -478,7 +488,15 @@ def update(self, variables): for i, (model, ys, inputs, var_pybamm) in enumerate( zip(self.all_models, self.all_ys, self.all_inputs, vars_pybamm) ): - if isinstance(var_pybamm, pybamm.ExplicitTimeIntegral): + if ys.size == 0 and self._exprtree_contains( + var_pybamm, pybamm.expression_tree.state_vector.StateVector + ): + raise KeyError( + f"Cannot process variable '{key}' as it was not part of the " + "solve. Please re-run the solve with `output_variables` set to " + "include this variable." + ) + elif isinstance(var_pybamm, pybamm.ExplicitTimeIntegral): cumtrapz_ic = var_pybamm.initial_condition cumtrapz_ic = cumtrapz_ic.evaluate() var_pybamm = var_pybamm.child diff --git a/tests/unit/test_solvers/test_idaklu_solver.py b/tests/unit/test_solvers/test_idaklu_solver.py index 604f559049..8423c16b68 100644 --- a/tests/unit/test_solvers/test_idaklu_solver.py +++ b/tests/unit/test_solvers/test_idaklu_solver.py @@ -550,21 +550,23 @@ def test_with_output_variables(self): # Construct a model and solve for all vairables, then test # the 'output_variables' option for each variable in turn, confirming # equivalence - - # construct model - model = pybamm.lithium_ion.DFN() - geometry = model.default_geometry - param = model.default_parameter_values input_parameters = {} # Sensitivities dictionary - param.update({key: "[input]" for key in input_parameters}) - param.process_model(model) - param.process_geometry(geometry) - var_pts = {"x_n": 50, "x_s": 50, "x_p": 50, "r_n": 5, "r_p": 5} - mesh = pybamm.Mesh(geometry, model.default_submesh_types, var_pts) - disc = pybamm.Discretisation(mesh, model.default_spatial_methods) - disc.process_model(model) t_eval = np.linspace(0, 3600, 100) + # construct model + def construct_model(): + model = pybamm.lithium_ion.DFN() + geometry = model.default_geometry + param = model.default_parameter_values + param.update({key: "[input]" for key in input_parameters}) + param.process_model(model) + param.process_geometry(geometry) + var_pts = {"x_n": 50, "x_s": 50, "x_p": 50, "r_n": 5, "r_p": 5} + mesh = pybamm.Mesh(geometry, model.default_submesh_types, var_pts) + disc = pybamm.Discretisation(mesh, model.default_spatial_methods) + disc.process_model(model) + return model + options = { "linear_solver": "SUNLinSol_KLU", "jacobian": "sparse", @@ -585,6 +587,24 @@ def test_with_output_variables(self): "Throughput capacity [A.h]", # ExplicitTimeIntegral ] + # vars that are not in the output_variables list, but are still accessible as + # they are either model parameters, or do not require access to the state vector + model_vars = [ + "Time [s]", + "C-rate", + "Ambient temperature [K]", + "Porosity", + ] + + # A list of variables that are not in the model and cannot be computed + inaccessible_vars = [ + "Terminal voltage [V]", + "Negative particle surface stoichiometry", + "Electrode current density [A.m-2]", + "Power [W]", + "Resistance [Ohm]", + ] + # Use the full model as comparison (tested separately) solver_all = pybamm.IDAKLUSolver( atol=1e-8, @@ -592,7 +612,7 @@ def test_with_output_variables(self): options=options, ) sol_all = solver_all.solve( - model, + construct_model(), t_eval, inputs=input_parameters, calculate_sensitivities=True, @@ -606,15 +626,20 @@ def test_with_output_variables(self): output_variables=output_variables, ) sol = solver.solve( - model, + construct_model(), t_eval, inputs=input_parameters, ) # Compare output to sol_all - for varname in output_variables: + for varname in [*output_variables, *model_vars]: self.assertTrue(np.allclose(sol[varname].data, sol_all[varname].data)) + # Check that the missing variables are not available in the solution + for varname in inaccessible_vars: + with self.assertRaises(KeyError): + sol[varname].data + # Mock a 1D current collector and initialise (none in the model) sol["x_s [m]"].domain = ["current collector"] sol["x_s [m]"].initialise_1D() From 8b5899dc2e8a6d86147ad8740b7b6a2d1567c6e3 Mon Sep 17 00:00:00 2001 From: John Brittain Date: Sat, 3 Feb 2024 00:26:59 +0000 Subject: [PATCH 2/4] Move expression tree 'contains' method to Symbol class --- pybamm/expression_tree/symbol.py | 12 ++++++++++++ pybamm/solvers/solution.py | 14 ++------------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/pybamm/expression_tree/symbol.py b/pybamm/expression_tree/symbol.py index 9c719159c7..4b8cf096a4 100644 --- a/pybamm/expression_tree/symbol.py +++ b/pybamm/expression_tree/symbol.py @@ -983,6 +983,18 @@ def shape_for_testing(self): else: return evaluated_self.shape + def contains(self, expr_type): + """ + Does the symbol tree contain a given node type? + """ + if type(self) is expr_type: + return True + elif "children" in dir(self): + for child in self.children: + if child.contains(expr_type): + return True + return False + @property def ndim_for_testing(self): """ diff --git a/pybamm/solvers/solution.py b/pybamm/solvers/solution.py index 43aba4b955..f635356384 100644 --- a/pybamm/solvers/solution.py +++ b/pybamm/solvers/solution.py @@ -457,16 +457,6 @@ def set_summary_variables(self, all_summary_variables): {name: np.array(value) for name, value in summary_variables.items()} ) - def _exprtree_contains(self, expr, expr_type): - """Does the expression tree contain a given type of node?""" - if type(expr) is expr_type: - return True - elif "children" in dir(expr): - for child in expr.children: - if self._exprtree_contains(child, expr_type): - return True - return False - def update(self, variables): """Add ProcessedVariables to the dictionary of variables in the solution""" # make sure that sensitivities are extracted if required @@ -488,8 +478,8 @@ def update(self, variables): for i, (model, ys, inputs, var_pybamm) in enumerate( zip(self.all_models, self.all_ys, self.all_inputs, vars_pybamm) ): - if ys.size == 0 and self._exprtree_contains( - var_pybamm, pybamm.expression_tree.state_vector.StateVector + if ys.size == 0 and var_pybamm.contains( + pybamm.expression_tree.state_vector.StateVector ): raise KeyError( f"Cannot process variable '{key}' as it was not part of the " From 30cb0ec17a4879ee1362569e68ff93e95c3148e8 Mon Sep 17 00:00:00 2001 From: John Brittain Date: Mon, 5 Feb 2024 10:13:46 +0000 Subject: [PATCH 3/4] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdacb00ae3..06a6184605 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ## Bug Fixes +- Update IDAKLU solver to fail gracefully when a variable is requested that was not in the solves `output_variables` list ([#3803](https://github.com/pybamm-team/PyBaMM/pull/3803)) - Updated `_steps_util.py` to throw a specific exception when drive cycle starts at t>0 ([#3756](https://github.com/pybamm-team/PyBaMM/pull/3756)) - Updated `plot_voltage_components.py` to support both `Simulation` and `Solution` objects. Added new methods in both `Simulation` and `Solution` classes for allow the syntax `simulation.plot_voltage_components` and `solution.plot_voltage_components`. Updated `test_plot_voltage_components.py` to reflect these changes ([#3723](https://github.com/pybamm-team/PyBaMM/pull/3723)). - The SEI thickness decreased at some intervals when the 'electron-migration limited' model was used. It has been corrected ([#3622](https://github.com/pybamm-team/PyBaMM/pull/3622)) From d36c9bfc78cfe20b19e5ff4385e03b07a48c7a45 Mon Sep 17 00:00:00 2001 From: John Brittain Date: Wed, 21 Feb 2024 09:27:10 +0000 Subject: [PATCH 4/4] Use existing method has_symbol_of_classes to test for dependence on state vector --- pybamm/expression_tree/symbol.py | 12 ------------ pybamm/solvers/solution.py | 2 +- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/pybamm/expression_tree/symbol.py b/pybamm/expression_tree/symbol.py index 4b8cf096a4..9c719159c7 100644 --- a/pybamm/expression_tree/symbol.py +++ b/pybamm/expression_tree/symbol.py @@ -983,18 +983,6 @@ def shape_for_testing(self): else: return evaluated_self.shape - def contains(self, expr_type): - """ - Does the symbol tree contain a given node type? - """ - if type(self) is expr_type: - return True - elif "children" in dir(self): - for child in self.children: - if child.contains(expr_type): - return True - return False - @property def ndim_for_testing(self): """ diff --git a/pybamm/solvers/solution.py b/pybamm/solvers/solution.py index f635356384..3fe4915dfb 100644 --- a/pybamm/solvers/solution.py +++ b/pybamm/solvers/solution.py @@ -478,7 +478,7 @@ def update(self, variables): for i, (model, ys, inputs, var_pybamm) in enumerate( zip(self.all_models, self.all_ys, self.all_inputs, vars_pybamm) ): - if ys.size == 0 and var_pybamm.contains( + if ys.size == 0 and var_pybamm.has_symbol_of_classes( pybamm.expression_tree.state_vector.StateVector ): raise KeyError(