From 53a1942745c139cb8e09c2e1b6228468061224b1 Mon Sep 17 00:00:00 2001 From: Robert Timms Date: Fri, 11 Jun 2021 15:41:39 +0100 Subject: [PATCH 1/2] #1507 remove algebraic equation check --- pybamm/models/base_model.py | 36 +++++------------------ tests/unit/test_models/test_base_model.py | 12 -------- 2 files changed, 8 insertions(+), 40 deletions(-) diff --git a/pybamm/models/base_model.py b/pybamm/models/base_model.py index 36bd5b862e..60e8fce18e 100644 --- a/pybamm/models/base_model.py +++ b/pybamm/models/base_model.py @@ -639,40 +639,20 @@ def check_well_determined(self, post_discretisation): def check_algebraic_equations(self, post_discretisation): """ - Check that the algebraic equations are well-posed. - Before discretisation, each algebraic equation key must appear in the equation - After discretisation, there must be at least one StateVector in each algebraic - equation + Check that the algebraic equations are well-posed. After discretisation, + there must be at least one StateVector in each algebraic equation. """ - vars_in_bcs = set() - unpacker = pybamm.SymbolUnpacker(pybamm.Variable) - for side_eqn in self.boundary_conditions.values(): - all_vars = unpacker.unpack_list_of_symbols( - [eqn for eqn, _ in side_eqn.values()] - ) - vars_in_bcs.update(all_vars.keys()) - if not post_discretisation: - # After the model has been defined, each algebraic equation key should - # appear in that algebraic equation, or in the boundary conditions - # this has been relaxed for concatenations for now - for var, eqn in self.algebraic.items(): - if not ( - any(x.id == var.id for x in eqn.pre_order()) - or var.id in vars_in_bcs - or isinstance(var, pybamm.Concatenation) - ): - raise pybamm.ModelError( - "each variable in the algebraic eqn keys must appear in the eqn" - ) - else: - # variables in keys don't get discretised so they will no longer match - # with the state vectors in the algebraic equations. Instead, we check - # that each algebraic equation contains some StateVector + if post_discretisation: + # Check that each algebraic equation contains some StateVector for eqn in self.algebraic.values(): if not eqn.has_symbol_of_classes(pybamm.StateVector): raise pybamm.ModelError( "each algebraic equation must contain at least one StateVector" ) + else: + # We do not perfom any checks before discretisation (most problematic + # cases should be caught by `check_well_determined`) + pass def check_ics_bcs(self): """Check that the initial and boundary conditions are well-posed.""" diff --git a/tests/unit/test_models/test_base_model.py b/tests/unit/test_models/test_base_model.py index 9ad1ce6cc4..dfe453771f 100644 --- a/tests/unit/test_models/test_base_model.py +++ b/tests/unit/test_models/test_base_model.py @@ -342,18 +342,6 @@ def test_check_well_posedness_variables(self): with self.assertRaisesRegex(pybamm.ModelError, "extra algebraic keys"): model.check_well_posedness() - # before discretisation, fail if the algebraic eqn keys don't appear in the eqns - model = pybamm.BaseModel() - model.algebraic = {c: d - 2, d: d - c} - with self.assertRaisesRegex( - pybamm.ModelError, - "each variable in the algebraic eqn keys must appear in the eqn", - ): - model.check_well_posedness() - # passes when we switch the equations around - model.algebraic = {c: d - c, d: d - 2} - model.check_well_posedness() - # after discretisation, algebraic equation without a StateVector fails model = pybamm.BaseModel() model.algebraic = { From e13300943caf6c5effc9236439f21b852f33a8a3 Mon Sep 17 00:00:00 2001 From: Robert Timms Date: Sat, 12 Jun 2021 09:41:46 +0100 Subject: [PATCH 2/2] #1507 changelog --- CHANGELOG.md | 1 + .../effective_resistance_current_collector.py | 2 +- .../current_collector/quite_conductive_potential_pair.py | 4 +--- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60fb615ad1..0d3c4e3679 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ ## Bug fixes +- Removed the overly-restrictive check "each variable in the algebraic eqn keys must appear in the eqn" ([#1510](https://github.com/pybamm-team/PyBaMM/pull/1510)) - Made parameters importable through pybamm ([#1475](https://github.com/pybamm-team/PyBaMM/pull/1475)) ## Breaking changes diff --git a/pybamm/models/submodels/current_collector/effective_resistance_current_collector.py b/pybamm/models/submodels/current_collector/effective_resistance_current_collector.py index 5765159fee..cde50f29e8 100644 --- a/pybamm/models/submodels/current_collector/effective_resistance_current_collector.py +++ b/pybamm/models/submodels/current_collector/effective_resistance_current_collector.py @@ -350,7 +350,7 @@ def __init__(self): f_p: pybamm.laplacian(f_p) - pybamm.source(1, f_p) + c * pybamm.DefiniteIntegralVector(f_p, vector_type="column"), - c: pybamm.yz_average(f_p) + pybamm.NotConstant(0) * c, + c: pybamm.yz_average(f_p), } # Boundary conditons diff --git a/pybamm/models/submodels/current_collector/quite_conductive_potential_pair.py b/pybamm/models/submodels/current_collector/quite_conductive_potential_pair.py index 4555c0e102..d82712786e 100644 --- a/pybamm/models/submodels/current_collector/quite_conductive_potential_pair.py +++ b/pybamm/models/submodels/current_collector/quite_conductive_potential_pair.py @@ -67,9 +67,7 @@ def set_algebraic(self, variables): * pybamm.laplacian(phi_s_cp) + pybamm.source(i_boundary_cc_0, phi_s_cp) + c * pybamm.PrimaryBroadcast(cc_area, "current collector"), - c: pybamm.Integral(i_boundary_cc, z) - - applied_current / cc_area - + pybamm.Multiplication(0, c), + c: pybamm.Integral(i_boundary_cc, z) - applied_current / cc_area, } def set_initial_conditions(self, variables):