Fix: half-cell DFN with surface form + lumped thermal fails to build (#5414) - #5540
Open
gaoflow wants to merge 3 commits into
Open
Fix: half-cell DFN with surface form + lumped thermal fails to build (#5414)#5540gaoflow wants to merge 3 commits into
gaoflow wants to merge 3 commits into
Conversation
gaoflow
force-pushed
the
fix/5414-half-cell-surface-form-phi-e-bc
branch
3 times, most recently
from
June 12, 2026 15:22
e5ff576 to
900a139
Compare
…ybamm-team#5414) In half-cell mode (`"working electrode": "positive"`) the planar negative electrode skips both the negative-domain electrolyte conductivity submodel and the base electrode-Ohm submodel's BC registration, leaving two key potentials with no boundary conditions: 1. `phi_e` (the concatenated electrolyte potential) -- registered only inside `BaseSurfaceFormConductivity.set_boundary_conditions`'s `self.domain == "negative"` branch. With no BC on the concatenation, `Discretisation.set_internal_boundary_conditions` cannot split it into per-orphan BCs, so `grad(phi_e_s)` (and `grad(phi_e_p)`) discretise to the internal mesh edges only. 2. `phi_s_p` (the porous positive electrode potential) -- the entire `base_ohm.BaseModel.set_boundary_conditions` method returned early the moment the negative electrode was planar, so the positive instance's BCs were never set even though the positive electrode is porous. In the surface form, `phi_s_p` is built as `-IndefiniteIntegral(i_s / sigma, x_p) + boundary_value(...)`, so the solid Ohmic-heating term `-inner(i_s_p, grad(phi_s_p))` and the electrolyte Ohmic-heating term `-inner(i_e_X, grad(phi_e_X))` both ended up trying to multiply the IndefiniteIntegral matrix (shape (n, n+1)) by internal-edge integrand vectors (shape (n-1, 1)), giving: ShapeError: Cannot find shape (original error: matmul: dimension mismatch with signature (n,k=21),(k=19,m)->(n,m)) Both BCs are now registered in the half-cell path: - The `phi_e` BC is moved into the separator branch of the full surface-form's `set_boundary_conditions` (which is always instantiated) and respects the half-cell Dirichlet convention used by `BaseElectrolyteConductivity`. - The base electrode-Ohm submodel's planar-skip is gated on the *current* submodel's domain rather than the negative electrode globally. Includes a regression test that builds and solves the failing `(working electrode positive, surface form, lumped thermal)` matrix on Xu2019, asserts both BC keys are present, and pins full-cell numerics as a non-regression check. Fixes pybamm-team#5414
gaoflow
force-pushed
the
fix/5414-half-cell-surface-form-phi-e-bc
branch
from
June 18, 2026 22:19
900a139 to
b74e6fa
Compare
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.
Problem
In half-cell mode (
{"working electrode": "positive"}) the planar Li-metal negative electrode causes two submodels to be skipped, both of which previously registered key boundary conditions:BaseSurfaceFormConductivity.set_boundary_conditionswas the only place that registered the inter-domain BCs onphi_e(the concatenated electrolyte potential). With no BC key on the concatenation,Discretisation.set_internal_boundary_conditionscannot split it into per-orphan BCs, sograd(phi_e_s)/grad(phi_e_p)end up internal-edge-only at discretisation time.base_ohm.BaseModel.set_boundary_conditionsreturned the moment the negative electrode was planar, silently dropping the BCs on the porous positive electrode potentialphi_s_ptoo.In the surface form,
phi_s_pis built as-IndefiniteIntegral(i_s / sigma, x_p) + boundary_value(...), so the solid Ohmic-heating term-inner(i_s_p, grad(phi_s_p))and the electrolyte Ohmic-heating term-inner(i_e_X, grad(phi_e_X))inbase_thermal.BaseThermal._get_standard_coupled_variablesboth ended up multiplying anIndefiniteIntegralmatrix of shape(n, n+1)by an internal-edge integrand of shape(n-1, 1). Building any DFN with this combination then crashed:The bug fires whenever all three of these are on:
Originally reported in #5414 with the standard
Xu2019parameter set and a DFN +"thermal": "lumped".Root cause
Two independent BC-registration sites silently no-op in the half-cell path:
src/pybamm/models/submodels/electrolyte_conductivity/surface_potential_form/full_surface_form_conductivity.py—phi_eBC is registered insideif self.domain == "negative":(lines 206–215 before this PR). Half-cell DFN skips the negative submodel viaif self.options.electrode_types.get(domain) == "planar": continue, so the BC is never added.src/pybamm/models/submodels/electrode/ohm/base_ohm.py—set_boundary_conditionsbails out as soon as the negative electrode is planar (if self.options.electrode_types["negative"] == "planar": return), instead of checking the current submodel's domain. So the positive instance — which is porous in a positive half-cell — also returns without registering itsphi_s_pBC.Fix
full_surface_form_conductivity.py: register thephi_eBC from the separator submodel'sset_boundary_conditionsinstead. The separator submodel is the one that is always instantiated for both full-cell and half-cell models. The BC values mirror the half-cell convention used byBaseElectrolyteConductivity.set_boundary_conditions:Dirichlet(phi_e_ref)on the left when the negative is planar,Neumann(0)otherwise;Neumann(0)on the right. Useupdate()rather than assignment to preserve thec_e_sNeumann BCs thatget_coupled_variablesset earlier in the same submodel's lifecycle.base_ohm.py: gate the early-return onself.options.electrode_types[self.domain] == "planar"instead of on the negative electrode globally. Now the planar submodel still skips (correctly — Li metal has no porousphi_sBC to set), and the porous positive submodel in a half-cell can register itsphi_s_pBCs the same way it does in a full-cell.Together these make the half-cell
set_internal_boundary_conditionssplit work the same way the full-cell one already does, andgrad(phi_s_p)/grad(phi_e_p)/grad(phi_e_s)all discretise to full edges.Verification
The reproducer from #5414 (DFN +
Xu2019+ missing thermal parameters + half-cell + surface form + lumped thermal) now builds and solves cleanly:A 12-entry triage matrix over
{DFN} × {full-cell, half-cell} × {surface form: false/algebraic/differential} × {thermal: isothermal/lumped}all build and solve. Full-cell numerics onChen2020are unchanged (V_end ≈ 3.51–3.53 Vacross the matrix, identical tomain).Targeted regression tests live in
tests/unit/test_models/test_submodels/test_electrolyte_conductivity/test_half_cell_surface_form_5414.pyand assert:phi_eis a BC key on the half-cell DFN (covers the surface-form fix).phi_s_pis a BC key on the half-cell DFN even with"surface form": "false"(covers thebase_ohmfix).Test impact
Scope
Three production files plus a new regression test:
src/pybamm/models/submodels/electrolyte_conductivity/surface_potential_form/full_surface_form_conductivity.py— move thephi_eBC into the separator branch and unify the half-cell vs full-cell BC values with the base-class convention.src/pybamm/models/submodels/electrode/ohm/base_ohm.py— make the planar-skip per-domain instead of per-model.CHANGELOG.md— unreleased bug-fix entry.tests/unit/test_models/test_submodels/test_electrolyte_conductivity/test_half_cell_surface_form_5414.py— new regression suite.No public API changes.