Skip to content

Use true node spacing for finite-volume node-to-edge shifts - #5755

Merged
rtimms merged 3 commits into
pybamm-team:mainfrom
rtimms:claude/fix-nonuniform-node-to-edge
Sep 17, 2026
Merged

rtimms merged 3 commits into
pybamm-team:mainfrom
rtimms:claude/fix-nonuniform-node-to-edge

Conversation

@rtimms

@rtimms rtimms commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #5754.

The bug

FiniteVolume.shift's node to edge matrices hard-code weights that assume
uniformly spaced nodes:

  • arithmetic_mean interpolates interior edges with [0.5, 0.5]
  • both arithmetic_mean and harmonic_mean extrapolate the two exterior edges
    with [1.5, -0.5] and [-0.5, 1.5]

boundary_value_or_flux extrapolates using the true node positions instead. On a
uniform mesh the two agree and nothing is wrong. On a non-uniform mesh they do
not, and that breaks mass conservation wherever a flux is imposed as a gradient
divided by a surface value — which is exactly what the Fickian particle does
(fickian_diffusion.py:209):

f"{Domain} {phase_name}particle bc [mol.m-4]": -j * R_nondim / self.param.F / pybamm.surf(D_eff)

The flux that condition produces is -D_eff * grad(c), and D_eff reaches the
surface edge through node_to_edge (harmonic, since the operand is a gradient).
When that value disagrees with surf(D_eff), the imposed surface flux is off by
their ratio — a systematic bias in the same direction on every step, so it
accumulates monotonically rather than averaging out.

With a concentration-dependent particle diffusivity and an
Exponential1DSubMesh, ORegan2022 gains 26 % of its particle lithium over
three 1C cycles
. Nothing reports a loss or gain; Total lithium in particles [mol] just drifts. In a long ageing run with LAM and SEI active it looks like a
degradation signal, and LLI [%] stops reconciling with the model's own loss
terms.

The fix

Use the real node spacing for both:

  • interior edges are interpolated with distance weights
    (d_edges[i+1] / (d_edges[i] + d_edges[i+1]) on node i), matching the
    reasoning already used for harmonic_mean's beta
  • the two exterior edges are extrapolated from the true node positions, shared
    by both means via a small exterior_edge_rows helper

Both reduce to the old hard-coded values on a uniform mesh, so uniform-mesh
results are bit-for-bit unchanged.

Result

Drop in Total lithium in particles [mol] over three 1C cycles, stock parameter
sets, SPMe (negative = lithium created):

parameter set r_n uniform, before/after exponential, before exponential, after
ORegan2022 16 -0.0000 -25.9351 -0.0000
ORegan2022 64 0.0000 -0.3540 -0.0000
Ecker2015 16 0.0000 0.4274 0.0000
Ecker2015 64 -0.0000 0.0244 0.0000
Chen2020 (constant D) 16 0.0000 0.0000 0.0000

Tests

Adds test_node_to_edge_exterior_matches_boundary_value, parametrised over both
means. On an Exponential1DSubMesh with a linear field it asserts that the
exterior edge values equal boundary_value and equal the exact field value
there, and that arithmetic reproduces the linear field at every edge. Against
the old weights this gives 1.0100 and 3.0013 where the exact values are 1 and 3.

Full unit suite passes. test_idaklu_jax.py has three pre-existing failures on
this machine (ProcessedVariable indexing a 0-d time array); they reproduce
identically on unmodified main.

Note on scope

The exterior-edge rows are what break conservation. The interior [0.5, 0.5]
weights are a separate first-order accuracy error on non-uniform meshes; I fixed
both since they share a cause, but I'm happy to split the interior change out if
you'd rather keep the PR to the conservation bug.

rtimms and others added 2 commits September 16, 2026 17:08
`FiniteVolume.shift`'s "node to edge" matrices hard-coded weights that
assume uniformly spaced nodes: `[0.5, 0.5]` on interior edges and
`[1.5, -0.5]` / `[-0.5, 1.5]` on the two exterior edges.

`boundary_value_or_flux` extrapolates from the true node positions, so on
a non-uniform mesh the two disagree. That breaks mass conservation
wherever a flux is imposed as a gradient divided by a surface value, as
the Fickian particle does, because the flux the gradient produces
multiplies by the node-to-edge diffusivity while the boundary condition
divided by `surf(D_eff)`. The resulting bias acts in the same direction
every step, so it accumulates: with a concentration-dependent particle
diffusivity on an `Exponential1DSubMesh`, `ORegan2022` gains 26% of its
particle lithium over three 1C cycles.

Weight interior edges by the true distances and share one true-position
extrapolation for the exterior edges between the arithmetic and harmonic
means. Both reduce to the old values on a uniform mesh, so uniform-mesh
results are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.24%. Comparing base (6e5baab) to head (0d77486).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5755      +/-   ##
==========================================
+ Coverage   98.15%   98.24%   +0.08%     
==========================================
  Files         341      341              
  Lines       33710    33715       +5     
==========================================
+ Hits        33088    33123      +35     
+ Misses        622      592      -30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rtimms
rtimms marked this pull request as ready for review September 17, 2026 10:36
@rtimms
rtimms requested a review from a team as a code owner September 17, 2026 10:36
@rtimms
rtimms enabled auto-merge (squash) September 17, 2026 14:33
@rtimms
rtimms merged commit 71d9ff4 into pybamm-team:main Sep 17, 2026
37 checks passed
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.

[Bug]: Lithium is not conserved with concentration-dependent particle diffusivity on a non-uniform particle mesh

2 participants