Skip to content

Solve the composite electrode SOH by bracketed rootfind - #5730

Draft
MarcBerliner wants to merge 3 commits into
claude/brent-nodefrom
claude/composite-esoh-bracketed
Draft

Solve the composite electrode SOH by bracketed rootfind#5730
MarcBerliner wants to merge 3 commits into
claude/brent-nodefrom
claude/composite-esoh-bracketed

Conversation

@MarcBerliner

Copy link
Copy Markdown
Member

Stacked on #5729, which adds pybamm.Brent. Review that first; this diff is against it.

What changes

Each stoichiometry was a variable in one nine-variable algebraic system handed to a Newton solver, which makes reaching a state a basin-of-attraction problem — worn and lithium-poor cells sat outside it.

Now each stoichiometry is an expression, found by inverting the open-circuit potential it sits on with a pybamm.Brent. LithiumIonParameters.U clips its stoichiometry and adds an asymptote, so every OCP diverges outside [0, 1]; every inversion therefore brackets a root and converges from any target — including targets outside the physical range, whose answer is non-physical but exact.

Nothing is left for a solver to iterate, so the stoichiometries are read back through one CasADi function rather than per-variable post-processing. The capacities reach that function as inputs, so only the parameters its graph substitutes can invalidate it: a caller ageing a cell reuses it, a caller swapping an OCP does not.

Measured against main

8000 targets — 1000 states of charge on [0, 1], 1000 voltages on [2.5, 4.2] V, three hysteresis directions, four wear states, through the public API:

main this branch
failures 147 / 8000 0 / 8000
build 12.2 ms 18.5 ms
first solve 58.9 ms 14.8 ms

main's failures are silent — every one returns a state violating lithium conservation rather than raising, concentrated at low SOC.

Repeat calls through a reused simulation, reading the stoichiometries back:

main this branch
new target 18.8 ms 7.2 ms
aged capacities 15.3 ms 15.6 ms
no reuse (cold) 75.3 ms 91.2 ms

Known limitations

  • Reading variables through ProcessedVariable is slower than main (roughly 4×). It converts and evaluates the whole nested rootfind once per variable; the evaluator bypasses that, but only for get_initial_stoichiometries_composite, which is the path Simulation uses. A caller driving ElectrodeSOHComposite directly pays it.
  • The build is ~1.5× slower: the Brent graph is larger symbolically than the algebraic system it replaces. Paid once per model.
  • solve_split is kept as the fallback and is now actually reachable — the native rootfinder reports failure as RuntimeError, which the old ladder did not catch, and the retry re-ran an identical solve. It now returns the split result instead.
  • Bracketing guarantees convergence, not uniqueness. A user OCP that is non-monotone within [0, 1] would still bracket, and Brent would return one of its roots.
  • Composite-positive coverage in the sweeps uses parameters aliased from their single-phase equivalents, since Chen2020_composite describes a composite negative. That is a degenerate composite, not a real parameter set.

Tests: 4221 unit, 32 sweep tests in test_electrode_soh_composite_sweeps.py.

🤖 Generated with Claude Code

@MarcBerliner
MarcBerliner requested a review from a team as a code owner August 20, 2026 13:59
@MarcBerliner
MarcBerliner marked this pull request as draft August 20, 2026 14:03
@MarcBerliner
MarcBerliner force-pushed the claude/composite-esoh-bracketed branch 4 times, most recently from 20626a7 to 238ea21 Compare August 20, 2026 17:29
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.46835% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.96%. Comparing base (fa12f8d) to head (3a8b1da).

Files with missing lines Patch % Lines
...tery_models/lithium_ion/electrode_soh_composite.py 97.41% 4 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           claude/brent-node    #5730      +/-   ##
=====================================================
- Coverage              98.08%   97.96%   -0.13%     
=====================================================
  Files                    341      341              
  Lines                  32916    32993      +77     
=====================================================
+ Hits                   32285    32320      +35     
- Misses                   631      673      +42     

☔ 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.

@MarcBerliner
MarcBerliner force-pushed the claude/composite-esoh-bracketed branch from 238ea21 to 48e3612 Compare August 21, 2026 16:23
@MarcBerliner
MarcBerliner force-pushed the claude/composite-esoh-bracketed branch from 48e3612 to f2c5d45 Compare August 21, 2026 16:43
MarcBerliner added a commit that referenced this pull request Aug 24, 2026
…ached

Windows holds two copies of CasADi -- ours built with MSVC, the wheel's with
MinGW -- so the plugin registered in one is invisible to the other, and the
node refused to convert there at all. That took composite electrode SOH in
#5730 with it, which works on Windows today.

A CasADi Callback closes the gap. The residual is still compiled into the
oracle exactly as the plugin path builds it; only the bracketing iteration
runs in Python, driven by scipy.optimize.brentq. Everything around the
rootfind stays in the graph, so a rootfind nested inside another costs one
callback per enclosing iteration rather than a Python tree walk per residual
evaluation: 200 nested solves take 0.26s this way against 0.004s through the
plugin, where evaluating the tree directly could not finish one composite SOH
solve in fourteen minutes.

Derivatives come from the implicit function theorem, as the plugin's do:
dx/dp = -(dF/dp) / (dF/dx) at the root, emitted as a forward mode over the
same oracle. The tests confirm both paths agree to 1e-9 on a case with a
known answer.

Two things the driver cannot do. It re-enters Python, so the test that pins
that out of the plugin path now skips when the plugin is absent, and it holds
a callback, so an expression containing one cannot be code-generated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MarcBerliner
MarcBerliner force-pushed the claude/composite-esoh-bracketed branch from f2c5d45 to d5d51d9 Compare August 24, 2026 16:25
MarcBerliner and others added 3 commits August 24, 2026 14:44
Each stoichiometry was a variable in one nine-variable algebraic system handed
to a Newton solver, which is a basin-of-attraction problem: worn and
lithium-poor states sat outside it. Now each is an expression, found by
inverting the open-circuit potential it sits on with a `pybamm.Brent`. The
OCPs diverge outside [0, 1], so every inversion brackets a root and converges
from any target, including targets outside the physical range, whose answer is
non-physical but exact.

Nothing is left for a solver to iterate, so the stoichiometries are read back
through one CasADi function rather than per-variable post-processing. The
capacities reach that function as inputs, so only the parameters its graph
substitutes can invalidate it: a caller ageing a cell reuses it, and a caller
swapping an open-circuit potential does not.

Over 8000 targets -- a thousand states of charge and a thousand voltages,
three hysteresis directions, four wear states:

                     main     branch
  failures         147/8000   0/8000
  build             12.2 ms   18.5 ms
  first solve       58.9 ms   14.8 ms

Main's failures are silent: it returns a state violating lithium conservation
rather than raising.

Repeat calls through a reused simulation, reading the stoichiometries back:

                     main     branch
  new target        18.8 ms    7.2 ms
  aged capacities   15.3 ms   15.6 ms
  no reuse          75.3 ms   91.2 ms

Reading the nine variables back through ProcessedVariable is still slower than
main, which the evaluator bypasses only for
get_initial_stoichiometries_composite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The full solve evaluates the stoichiometries as one expression holding
rootfinds nested about three deep -- one composite solve makes on the order of
a million of them -- which only the native "brent" plugin gets through quickly.
Windows cannot load that plugin, and neither Python-side route is usable there:
walking the tree could not finish a single solve in fourteen minutes, and
driving the compiled residual from a CasADi callback still pays 830k Python
round trips.

So Windows takes the path it took before this branch. solve_split is algebraic
throughout -- _ElectrodeSOH through a Simulation, no rootfind node anywhere in
it -- and get_initial_stoichiometries_composite already falls back to it, so
raising from solve_full is all it takes. 47 of the 63 tests in the file pass
that way in 35s; the 16 that exercise the full solve itself skip, since what
they test is genuinely absent.

pybamm.is_windows() joins is_macos_intel() in util, so the check has one home.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_reaches_the_requested_state solves ElectrodeSOHComposite as a model and
reads its variables back, so the rootfinds travel into IDAKLU as a serialised
function. On Windows they are a CasADi Callback by then, which the solver's own
CasADi cannot deserialise -- "not found 'CallbackInternal'" -- so this is the
same gap as the other full-solve tests, one layer further in.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MarcBerliner
MarcBerliner force-pushed the claude/composite-esoh-bracketed branch from d5d51d9 to 3a8b1da Compare August 24, 2026 18:47
@MarcBerliner
MarcBerliner marked this pull request as ready for review August 24, 2026 20:53

@aabills aabills left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

real review pending #5729

Q_p.append(pybamm.InputParameter("Q_p_2"))

T_ref = param.T_ref
T_init = param.T_init if initialization_method == "voltage" else T_ref

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this logic necessary? Kinda strange that we change the value of temperature based on initialization method

@MarcBerliner
MarcBerliner marked this pull request as draft September 3, 2026 20:07
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