search

pybamm-team / PyBaMM

Fast and flexible physics-based battery models in Python
https://www.pybamm.org/
BSD 3-Clause "New" or "Revised" License
1.12k stars 547 forks source link

[Bug]: Binary operators on concatenations assume that each concatenation has the same number of orphans #4561

Closed aabills closed 1 week ago

aabills commented 2 weeks ago

PyBaMM Version

develop

Python Version

3.12

Describe the bug

If a binary operator, such as +, is invoked with 2 concatenations (i.e. isinstance(left, pybamm.Concatenation) and isinstance(right, pybamm.Concatenation) == True), and the concatenations have a different number of orphans, the "extra orphans" are dropped. This can be easily seen here.

This is causing reaction heating to not be present in DFN half-cell models (see comments in #4557)

Steps to Reproduce

no MWE yet, but should be self evident in the code.

Relevant log output

No response

aabills commented 2 weeks ago

I think there are two different options for "solving" this:

1) Raise an error when this situation occurs 2) Pass the identity of the "unmatched orphans".

I implemented (1) to check how impactful it will be, and the only test that fails is the specific case I found this bug in: Half-cell DFN models. Therefore, I am of the opinion that we should implement (1) and fix the underlying issue that causes it in the half-cell case.

rtimms commented 2 weeks ago

On board with option 1.