Skip to content

fix(idaklu): hand IDAS each parameter's magnitude as pbar - #5783

Merged
BradyPlanden merged 1 commit into
mainfrom
a2b/idas-sensitivity-scales
Sep 25, 2026
Merged

BradyPlanden merged 1 commit into
mainfrom
a2b/idas-sensitivity-scales

Conversation

@BradyPlanden

Copy link
Copy Markdown
Member

Description

IDAKLUSolver computes forward sensitivities with IDAS but never called IDASetSensParams, so every sensitivity scale pbar stayed at 1. IDAS holds the scaled sensitivity pbar * dy/dp to the state tolerances. For a parameter as small as Chen2020's positive particle diffusivity (4e-15), dy/dp is around 1e14, and the unscaled column breaks the Newton convergence test through rounding alone. A DFN solve for that sensitivity fails with IDA_CONV_FAIL at every rtol = atol from 1e-8 to 1e-12. At looser tolerances it survives many convergence failures but runs 4.7x slower than it needs to at the defaults.

This PR passes pbar = |p| per differentiated parameter and input set, which makes the error weighting independent of the parameter's units. Measured against unit scales on the same build:

Case pbar = 1 (main) → pbar = |p|
DFN, positive particle diffusivity, rtol = atol 1e-8 to 1e-12 IDA_CONV_FAIL → converges, gradient within 3e-7 of finite differences
same, default tolerances 86 ms → 18 ms
DFN, both diffusivities and the current together, default tolerances 309 ms → 58 ms
SPMe, positive particle diffusivity, default tolerances 2.0x faster
Parameters near 1, such as the applied current within 10%
Large parameters (a 1000 mol.m-3 concentration, 298 K), default tolerances 18-19 ms → 29-30 ms, same accuracy

The last row is the cost: the sensitivity to a large parameter is now held to the state tolerances as well, which is up to 1.6x slower at the defaults (and faster at tight tolerances, where it is also more accurate).

For pybammsolvers maintainers: solve gains a keyword-only pbar argument after logger, so existing positional callers are unaffected. Because pybamm always passes it, this bumps pybammsolvers to 0.10.0 and pins pybammsolvers>=0.10.0,<0.11.0. #5782 (solver statistics) makes the identical version bump.

  • pybamm builds one row of |p| per input set, in sensitivity-column order, and passes it to the solver group.
  • The binding validates pbar's dimensions, rows and columns (raising ValueError). An empty pbar keeps IDAS's unit default.
  • Each solver clamps zero or non-finite scales to 1, since IDAS rejects a zero pbar, and re-applies the scales after every IDASensReInit, which resets them.
  • Tests cover the per-set rows, a model solved at two very different parameter magnitudes in one call (identical steps and scaled sensitivities), and a tight-tolerance DFN diffusivity gradient against central finite differences.

This is one of the backend-neutral pre-flight PRs split out of the Rust execution engine (#5732). It has no Rust dependency. It overlaps #5782 (solver statistics) in IDAKLUSolverGroup.cpp and the IDAKLU tests, so whichever lands second needs a rebase.

Type of change

Add an entry under # [Unreleased] in CHANGELOG.md, in one of ## Breaking changes, ## Deprecated, ## Features, or ## Bug fixes (include PR number; breaking and deprecation entries need a one-line migration note). Internal-only PRs — refactor, docs, CI, tests — can skip this. See RELEASE.md for the full policy.

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@BradyPlanden
BradyPlanden requested a review from a team as a code owner September 23, 2026 18:53
IDASetSensParams with each differentiated parameter's magnitude, one row per
input set through a keyword-only pbar argument after logger. Bumps
pybammsolvers to 0.10.0, which pybamm now requires.
@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.15%. Comparing base (2b6021d) to head (8104193).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5783   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files         341      341           
  Lines       33902    33908    +6     
=======================================
+ Hits        33275    33281    +6     
  Misses        627      627           

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

@martinjrobins martinjrobins 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.

thanks @BradyPlanden, this all looks good to me!

@BradyPlanden
BradyPlanden merged commit 15da388 into main Sep 25, 2026
69 of 71 checks passed
@BradyPlanden
BradyPlanden deleted the a2b/idas-sensitivity-scales branch September 25, 2026 10:15
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