Repository navigation
fix: sum the electrochemical heat sources over every particle phase - #5770
Merged
Merged
Conversation
`BaseThermal._get_standard_coupled_variables` builds `Q_rxn`, `Q_rev` and
`Q_hys` by zipping the phase names against the `"open-circuit potential"`
option. That option stays a plain string when it is left at its default, so
`[ocp_options]` had length one and
zip(["primary ", "secondary "], ["single"], strict=False)
silently truncated the loop after the primary phase. On a composite electrode
the secondary phase contributed no irreversible, reversible or hysteresis heat
at all.
A Chen2020 graphite electrode split into two identical half-volume-fraction
phases reported 13657 W.m-3 of volume-averaged irreversible heating against the
24371 W.m-3 of the equivalent single-phase electrode, and its lumped cell
temperature ran 2 K cold over 1200 s. Passing the option once per phase, as
`(("single", "single"), "single")`, was a workaround and gave the right answer.
Padding a scalar option out to the phase count is the whole fix; the zips stay
non-strict, so the one remaining length mismatch — two open-circuit potential
options declared against a single particle phase, which the options validator
accepts today — keeps being ignored rather than raising from inside the thermal
submodel.
The bug dates to #4893. Found while reviewing #5769, which fixes the same class
of omission in the heat of mixing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5770 +/- ##
==========================================
- Coverage 98.15% 98.15% -0.01%
==========================================
Files 341 341
Lines 33787 33787
==========================================
- Hits 33165 33164 -1
- Misses 622 623 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rtimms
approved these changes
Sep 22, 2026
mleot
added a commit
to mleot/PyBaMM
that referenced
this pull request
Sep 25, 2026
The composite heat-of-mixing tests only built the model. Extend the graphite/graphite heat-source comparison from pybamm-team#5770 so that splitting one graphite phase into two identical halves must also reproduce the one-phase "Volume-averaged heat of mixing [W.m-3]": - run it with "heat of mixing": "true", for isothermal (heat source only) and lumped thermal models, comparing the temperature rise for the latter - give Chen2020 the Ai2020 entropic changes; with zero dU/dT the reversible heat and the temperature dependence of U inside the heat of mixing were compared as zero against zero - solve at rtol 1e-6: at the default rtol the ~300 K temperature state drifts by ~0.03 K, which is 1% of the rise and failed SPM + lumped A heat of mixing that sums only the primary phase fails all four cases. Add a Si/graphite smoke test (Chen2020_composite, lumped) checking the heat of mixing stays positive and heats the cell. It supplies the per-layer "Negative electrode density [kg.m-3]" the set lacks and swaps the cubic Interpolant graphite OCP for the analytic Chen2020 fit, whose derivative the solver can convert. Also look up the electrode and phase parameters through param.domain_params and options.phases instead of getattr. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
BaseThermal._get_standard_coupled_variablesbuilds the irreversible, reversible and hysteresis heat sources by zipping the phase names against the"open-circuit potential"option. That option stays a plain string when it is left at its default, so[ocp_options]had length one andsilently truncated the loop after the primary phase. On a composite electrode the secondary phase contributed no
Q_rxn,Q_revorQ_hysat all.A Chen2020 graphite electrode split into two identical half-volume-fraction phases — physically the same electrode — reported 13657 W.m-3 of volume-averaged irreversible heating against the 24371 W.m-3 of the equivalent single-phase electrode, and its lumped cell temperature ran 2 K cold over 1200 s. Declaring the option once per phase, as
(("single", "single"), "single"), was a workaround and gave the right answer, which is what isolated the cause.The bug dates to #4893. Found while reviewing #5769, which fixes the same class of omission in the heat of mixing — that PR is correct as it stands and needs nothing from this one.
Fixes # (issue)
Type of change
Add an entry under
# [Unreleased]in CHANGELOG.md, in one of## Breaking changes,## Deprecated,## Features, or## Bug fixes(include PR number; breaking and deprecation entries need a one-line migration note). Internal-only PRs — refactor, docs, CI, tests — can skip this. See RELEASE.md for the full policy.Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests