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

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

Closed
aabills opened this issue Nov 4, 2024 · 2 comments · Fixed by #4562
Assignees
Labels
bug Something isn't working

Comments

@aabills
Copy link
Contributor

aabills commented Nov 4, 2024

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 aabills added the bug Something isn't working label Nov 4, 2024
@aabills aabills self-assigned this Nov 4, 2024
@aabills
Copy link
Contributor Author

aabills commented Nov 4, 2024

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
Copy link
Contributor

rtimms commented Nov 4, 2024

On board with option 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants