Repository navigation
Use true node spacing for finite-volume node-to-edge shifts - #5755
Merged
rtimms merged 3 commits intoSep 17, 2026
Merged
Conversation
`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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
rtimms
marked this pull request as ready for review
September 17, 2026 10:36
MarcBerliner
approved these changes
Sep 17, 2026
rtimms
enabled auto-merge (squash)
September 17, 2026 14:33
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.
Fixes #5754.
The bug
FiniteVolume.shift'snode to edgematrices hard-code weights that assumeuniformly spaced nodes:
arithmetic_meaninterpolates interior edges with[0.5, 0.5]arithmetic_meanandharmonic_meanextrapolate the two exterior edgeswith
[1.5, -0.5]and[-0.5, 1.5]boundary_value_or_fluxextrapolates using the true node positions instead. On auniform 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):The flux that condition produces is
-D_eff * grad(c), andD_effreaches thesurface 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 bytheir 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,ORegan2022gains 26 % of its particle lithium overthree 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 adegradation signal, and
LLI [%]stops reconciling with the model's own lossterms.
The fix
Use the real node spacing for both:
(
d_edges[i+1] / (d_edges[i] + d_edges[i+1])on nodei), matching thereasoning already used for
harmonic_mean'sbetaby both means via a small
exterior_edge_rowshelperBoth 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 parametersets, SPMe (negative = lithium created):
Tests
Adds
test_node_to_edge_exterior_matches_boundary_value, parametrised over bothmeans. On an
Exponential1DSubMeshwith a linear field it asserts that theexterior edge values equal
boundary_valueand equal the exact field valuethere, and that
arithmeticreproduces the linear field at every edge. Againstthe old weights this gives 1.0100 and 3.0013 where the exact values are 1 and 3.
Full unit suite passes.
test_idaklu_jax.pyhas three pre-existing failures onthis machine (
ProcessedVariableindexing a 0-d time array); they reproduceidentically 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.