Skip to content

fix: broadcast the average SEI film overpotential with a size distribution - #5892

Open
shaurya20335 wants to merge 4 commits into
pybamm-team:mainfrom
shaurya20335:fix-average-sei-film-size-distribution
Open

shaurya20335 wants to merge 4 commits into
pybamm-team:mainfrom
shaurya20335:fix-average-sei-film-size-distribution

Conversation

@shaurya20335

Copy link
Copy Markdown
Contributor

Description

With "SEI film resistance": "average" and "particle size": "distribution", SPMe, DFN and Newman-Tobias fail to build:

import pybamm
pybamm.lithium_ion.DFN(
    {"particle size": "distribution", "SEI": "constant", "SEI film resistance": "average"}
)
DomainError: children must have same or empty domains, not ['negative particle size'] and ['current collector']

In BaseKinetics, the average film drop eta_sei = -j_tot_av * L_sei_av * R_sei lives on the current collector. When the size distribution is resolved through the electrode, eta_r is on the particle-size domain with the electrode as its secondary domain, so eta_r += eta_sei cannot broadcast across the missing electrode level. MPM is not affected because its eta_r already has the current collector as its secondary domain, and "distributed" is not affected because its eta_sei is on the electrode.

eta_sei is now broadcast onto the electrode in that case. Nothing changes for models that already built: MPM and DFN without a distribution give bit-identical voltages with the average film.

With the fix (OKane2022, "SEI": "constant", 50 nm film, 5 A, voltage at 600 s):

model no film average film distributed film
DFN with size distribution 3.80096 V 3.78611 V 3.78574 V
SPMe with size distribution 3.80342 V 3.78859 V 3.78773 V
DFN (reference, no distribution) 3.80921 V 3.79431 V 3.79304 V

The average and distributed films differ by about 1 mV with a size distribution, as they do without one (1.5 mV for the DFN).

I found this while checking #5841 across model options.

Type of change

Bug fix, with an entry under ## Bug fixes in CHANGELOG.md.

Important checks:

  • No style issues: ruff 0.16.10 format and check, and comment-slop, pass on the changed files
  • Tests added that prove fix is effective: test_well_posed_psd_sei_film_resistance (unit, in the shared lithium-ion base tests) builds each model with a size distribution and both film options. It fails on main for SPMe, DFN and Newman-Tobias and passes with the fix.
  • All tests pass: the full unit suite (4,852 passed, 89 skipped) and the full integration suite (518 passed, 29 skipped) pass locally on macOS with Python 3.12. I am relying on CI for the notebooks, example scripts and other platforms.
  • The documentation builds: not run locally, no docs changes
  • Code is commented for hard-to-understand areas

…ution

With "SEI film resistance": "average", eta_sei is the x-averaged film drop on
the current collector. With "particle size": "distribution" resolved through
the electrode (SPMe, DFN, Newman-Tobias), eta_r lives on the particle-size
domain with the electrode as its secondary domain, so adding eta_sei raised
DomainError: children must have same or empty domains, not
['negative particle size'] and ['current collector']. Broadcast eta_sei onto
the electrode in that case. Models that already built are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shaurya20335
shaurya20335 requested a review from a team as a code owner October 12, 2026 03:35
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rtimms

rtimms commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Looks correct, thanks. I checked it together with #5841 (SEI film heating) and #5875: every size-distribution model builds with no, average or distributed film, and with #5841 the "average" film's extra heat matches its extra electrical loss to 0.13% (SPMe) and 0.01% (DFN) with a size distribution.

Heads-up on #5818, which deprecates relying on options set from other options (the test suite turns that warning into an error): test_well_posed_psd_sei_film_resistance fails with it for all 4 models, because the film options set "total interfacial current density as a state": "true" by default. Whichever of this PR and #5818 merges second needs to pass that option explicitly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shaurya20335

Copy link
Copy Markdown
Contributor Author

Thanks for checking it with #5841 and #5875, @rtimms. test_well_posed_psd_sei_film_resistance now passes "total interfacial current density as a state" explicitly ("false" for the average film and "true" for the distributed film), so it should not depend on the order this and #5818 merge in.

…m-size-distribution

# Conflicts:
#	CHANGELOG.md

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants