Skip to content

Pass the cache to nlsolve! in PDIRK44's out-of-place branch - #4352

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
singhharsh1708:pdirk-serial-threaded
Aug 26, 2026
Merged

Pass the cache to nlsolve! in PDIRK44's out-of-place branch#4352
ChrisRackauckas merged 1 commit into
SciML:masterfrom
singhharsh1708:pdirk-serial-threaded

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Aug 23, 2026

Copy link
Copy Markdown
Member

PDIRK44's out-of-place branch passes γ * dt where nlsolve! takes the
integrator cache:

k11 = nlsolve!(_nlsolver, integrator, γs[1] * dt, repeat_step)

The signature is nlsolve!(nlsolver, integrator, cache = nothing, repeat_step = false).
Four call sites do this, all in that one branch. Every other one of the 74
nlsolve! calls in the library passes the cache, and the in-place branch of the
same function passes it correctly, so this looks like a slip rather than
something deliberate.

Nothing complained, for two reasons that compound. The guard

cache === nothing && throw(ArgumentError("cache is not passed to `nlsolve!` when using NLNewton"))

only rejects nothing, and a Float64 sails through it. Then update_W! on the
out-of-place path routes to calc_W(integrator, nlsolver, dtgamma, repeat_step),
which does not read that argument at all. So the wrong value is carried down and
then ignored, and results are unaffected today.

This fixes the four call sites, and tightens the guard to check the type rather
than only nothing so the next one of these is caught where it happens instead
of being carried silently.

Testing

lib/OrdinaryDiffEqPDIRK/test/nlsolve_argument_tests.jl checks that the guard
rejects a non-cache third argument, and that the out-of-place and in-place paths
agree to rtol = 1e-10 with threading both on and off. The guard test fails on
the current code and passes with this change; the agreement test passes either
way, since the old bug was silent, and is there to pin the behaviour.

OrdinaryDiffEqPDIRK Core and OrdinaryDiffEqNonlinearSolve suites pass.

One unrelated thing seen while running these, not touched here:
OrdinaryDiffEqPDIRK's QA group fails on Sequential, BaseThreads and
PolyesterThreads being reported as stale imports. They are imported only to be
named in an Expr(:public, ...) declaration, which ExplicitImports does not
count as a use. It fails identically on an unmodified master, so it is not from
this change and wants its own decision about whether the public declaration or
the import should go.

Part of a set of three independent fixes in this area: #4345, #4351 and #4352.
They touch different code and do not overlap, but they bump some of the same
Project.toml version lines. Each bumps one patch from master rather than
reserving a number, since #4353 has just had to undo version skips for General's
AutoMerge, so whichever lands second needs its version line refreshed.

AI Disclosure

Claude assisted with this change.

@ChrisRackauckas
ChrisRackauckas merged commit e4f0c5e into SciML:master Aug 26, 2026
144 of 174 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.

2 participants