Skip to content

fix: validate solver inputs, options and model formats - #5784

Merged
BradyPlanden merged 6 commits into
mainfrom
a3/solver-inputs-options
Sep 29, 2026
Merged

BradyPlanden merged 6 commits into
mainfrom
a3/solver-inputs-options

Conversation

@BradyPlanden

Copy link
Copy Markdown
Member

Description

Several solver inputs could be wrong without anyone noticing:

  • model.convert_to_format was a plain attribute, so a typo was accepted and surfaced at solve time, if at all.
  • JSON save/load dropped convert_to_format, so a model saved as "python" came back as "casadi".
  • IDAKLUSolver and NonlinearSolver merged unknown options keys through, so {"num_thread": 4} silently kept the defaults.
  • Each solver carried its own copy of the input-stacking and flattening code.

Consolidating that code turned up several bugs. Restarting a sensitivity solve from an output_variables solve seeded the state sensitivities from the output sensitivities: repeated step calls failed with an unrelated shape error or returned wrong gradients, and an SPM discharge-then-rest Experiment returned a voltage sensitivity 19% below finite differences. Several input sets with nproc=1 went through a one-process multiprocessing pool, which ran the same solves in the same order as a loop but spawned an interpreter and pickled the model and every solution. For a 32-set DFN with calc_ic=False, finding consistent initial conditions took 1.8 s instead of 3 ms, and on macOS and Windows an unguarded script hung. There were also three smaller bugs: a per-state atol array crashed the root-solver setup, AlgebraicSolver's "minimize" gradient was wrong for non-CasADi formats, and the initial event check raised UnboundLocalError for convert_to_format=None.

Models and simulations pickled by earlier versions still load, and JSON files written by earlier versions load with the model's default format.

  • BaseModel.convert_to_format becomes a validated property that raises OptionError, with a __setstate__ migration for old pickles and a uses_stacked_inputs flag that names the input convention rather than the format.
  • The JSON serialisers write and read convert_to_format.
  • Shared stack_inputs/flatten_inputs helpers in base_solver.py replace the per-solver copies.
  • BaseSolver._overlay_options rejects unknown option keys, and IDAKLUSolver and NonlinearSolver use it.
  • BaseSolver._integrate loops in the calling process when nproc == 1.
  • step() rejects a sensitivity restart from an outputs-only solution with a SolverError. In an Experiment this ends the experiment after its first step, as other step failures do.
  • Fixes for the root-solver atol, the "minimize" gradient and the None-format event check.

This is one of the backend-neutral pre-flight PRs split out of the Rust execution engine (#5732). It has no Rust dependency.

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 19:01
Validate and serialise model.convert_to_format, reject unknown IDAKLU options and sensitivity restarts from output-only solves, and share the input-stacking helpers across solvers.
Also run nproc=1 multi-input solves in process and fix the per-state root-solver atol, the AlgebraicSolver minimize gradient and the format-None event check.
@BradyPlanden
BradyPlanden force-pushed the a3/solver-inputs-options branch from a54e604 to 92dc5d7 Compare September 23, 2026 19:02
@codecov

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.00%. Comparing base (2677897) to head (d41088c).

Files with missing lines Patch % Lines
...ackages/pybamm/src/pybamm/solvers/idaklu_solver.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5784   +/-   ##
=======================================
  Coverage   98.00%   98.00%           
=======================================
  Files         346      346           
  Lines       35259    35300   +41     
=======================================
+ Hits        34554    34595   +41     
  Misses        705      705           

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

Napoleon rendered the class docstring's Attributes entry as a second
description of the new property, which fails the -W docs build.

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

looks great, but I don't think convert_to_format = None is valid? this is just the same as , say, convert_to_format = 'asdf', which you are erroring on.

# Conflicts:
#	CHANGELOG.md
#	packages/pybamm/src/pybamm/solvers/idaklu_solver.py
None has evaluated as "python" since 2022, not by keeping the expression
tree as documented. Setting it now raises a DeprecationWarning and keeps
its current behaviour until removal.
@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

ProjectPyBaMM
Brancha3/solver-inputs-options
Testbedbare-metal

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 1 Alert

🐰 View full continuous benchmarking report in Bencher

@BradyPlanden
BradyPlanden enabled auto-merge (squash) September 29, 2026 08:12
@BradyPlanden
BradyPlanden merged commit aea1aab into main Sep 29, 2026
42 checks passed
@BradyPlanden
BradyPlanden deleted the a3/solver-inputs-options branch September 29, 2026 08:33
@rtimms rtimms mentioned this pull request Oct 9, 2026
rtimms added a commit that referenced this pull request Oct 9, 2026
Move the Unreleased entries under v26.10.0.0 and bump CITATION.cff. Move the
#5781, #5784, #5813 and #5817 entries out of v26.9.0.0, which was tagged
before they merged, and add the missing pybammsolvers>=0.11.0 breaking change.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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