Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw an error when concatenations have different number of children #4562

Merged
merged 13 commits into from
Nov 12, 2024
17 changes: 11 additions & 6 deletions src/pybamm/expression_tree/binary_operators.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,12 +856,17 @@ def _simplified_binary_broadcast_concatenation(
elif isinstance(right, pybamm.Concatenation) and not isinstance(
right, pybamm.ConcatenationVariable
):
return left.create_copy(
[
operator(left_child, right_child)
for left_child, right_child in zip(left.orphans, right.orphans)
]
)
if len(left.orphans) == len(right.orphans):
return left.create_copy(
[
operator(left_child, right_child)
for left_child, right_child in zip(left.orphans, right.orphans)
]
)
else:
raise AssertionError(
"Concatenations must have the same number of children"
)
if isinstance(right, pybamm.Concatenation) and not isinstance(
right, pybamm.ConcatenationVariable
):
Expand Down
10 changes: 10 additions & 0 deletions src/pybamm/models/full_battery_models/base_battery_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,16 @@ def options(self, extra_options):
f"must use surface formulation to solve {self!s} with hydrolysis"
)

if isinstance(self, pybamm.lithium_ion.DFN):
if options["working electrode"] == "positive":
if (
options["thermal"] == "lumped"
kratman marked this conversation as resolved.
Show resolved Hide resolved
or options["calculate heat source for isothermal models"] == "true"
):
raise pybamm.OptionError(
"Due to a known bug, half-cell models are not currently compatible with thermal models."
)

self._options = options

def set_standard_output_variables(self):
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/test_expression_tree/test_concatenations.py
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,18 @@ def test_to_from_json(self, mocker):

# test _from_json
assert pybamm.NumpyConcatenation._from_json(np_json) == conc_np

def test_same_number_of_children(self):
a = pybamm.Variable("y", domain=["1", "2"])
b = pybamm.Variable("z", domain=["3"])

d1 = pybamm.Variable("d1", domain=["1"])
d2 = pybamm.Variable("d2", domain=["2"])
d3 = pybamm.Variable("d3", domain=["3"])

d_concat = pybamm.concatenation(pybamm.sin(d1), pybamm.sin(d2), pybamm.sin(d3))
a_concat = pybamm.concatenation(pybamm.sin(a), pybamm.sin(b))
with pytest.raises(
AssertionError, match="Concatenations must have the same number of children"
):
a_concat + d_concat
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# Base unit tests for lithium-ion half-cell models
# This is achieved by using the {"working electrdode": "positive"} option
#
import pybamm
import pytest


class BaseUnitTestLithiumIonHalfCell:
Expand Down Expand Up @@ -81,8 +83,16 @@ def test_well_posed_asymmetric_ec_reaction_limited_sei(self):

def test_well_posed_lumped_thermal(self):
options = {"thermal": "lumped"}
self.check_well_posedness(options)
if self.model == pybamm.lithium_ion.DFN:
with pytest.raises(pybamm.OptionError):
self.check_well_posedness(options)
else:
self.check_well_posedness(options)

def test_well_posed_lumped_thermal_hom(self):
options = {"thermal": "lumped", "heat of mixing": "true"}
self.check_well_posedness(options)
if self.model == pybamm.lithium_ion.DFN:
with pytest.raises(pybamm.OptionError):
self.check_well_posedness(options)
else:
self.check_well_posedness(options)