NonlinearSolveAlg: let inner solver refresh J when no W is reused (fixes master nlstep order failures)#3818
Draft
ChrisRackauckas-Claude wants to merge 1 commit into
Conversation
SciML#3703 changed compute_step! from recompute_jacobian = cache.W === nothing || (iter == 1 && cache.new_W) to recompute_jacobian = iter == 1 && (cache.W === nothing || cache.new_W) which freezes the inner NonlinearSolve Jacobian after the first outer iteration on the paths without a reusable W (MTK nlstep, DAE, OOP). The inner solve silently degrades to modified Newton while the outer η·ndz < κ termination keeps its full-Newton calibration, so each step carries an unconverged remainder independent of dt. Observable: the FBDF + NSA convergence order on the MTK Robertson nlstep tests collapsed from ~1.26/1.30 to 0.615 (l∞ error stops halving at dt = 2^-18), failing both order assertions on master. Restore the pre-SciML#3703 condition for the W === nothing paths. The W-reuse (modified-Newton) path is unchanged: with a reused W the inner Jacobian is the ODE-side W by construction, so per-iteration refreshes remain pointless there. A/B on master: reverting only this condition turns test/modelingtoolkit/nlstep_tests.jl from 17 pass / 2 fail to 19 pass / 0 fail, with orders 1.2576 (< 0.3 bound) and 1.3035 (< 0.35 bound), and restores clean error halving under dt refinement. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Contributor
Author
|
CI triage: the failing lanes here are master-baseline, not from this change — StochasticDiffEq QA/downgrade + GPU lanes fail on all concurrent PRs including the 2-line #3816; the OrdinaryDiffEqNonlinearSolve Core lane failure is the pre-existing |
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 two failing tests on master
test/modelingtoolkit/nlstep_tests.jlcurrently fails on master (17 pass / 2 fail / 6 broken): the FBDF + NSA(TrustRegion) Robertson order assertions at lines 60 and 116 measure order 0.6152 against bounds|𝒪est−1| < 0.3 / 0.35. (#3807 read this as "order 1.38" — it's1 + 0.3848in the test output; the order collapsed below 1.)Root cause
#3703 changed both
compute_step!methods fromto
which freezes the inner NonlinearSolve Jacobian after outer iteration 1 on the paths without a reusable W (MTK nlstep, DAE, OOP). The inner solve silently degrades to modified Newton while the outer
η·ndz < κtermination keeps its calibration, so every step carries a dt-independent unconverged remainder. Measured l∞ errors overdts = 2^-15…2^-18: master[2.22e-7, 1.12e-7, 5.60e-8, 6.18e-8](error stops halving, then rises); with this fix[…, 1.38e-7, 5.60e-8, 2.80e-8](clean halving). Warmed TrustRegion and NewtonRaphson give bit-identical trajectories — not TR-specific, and consistent with the principle that a converged inner solve must not change the ODE solution.Fix
Restore the pre-#3703 condition:
W === nothingpaths let the inner solver refresh J every outer iteration; the W-reuse (modified-Newton) path is untouched — with a reused W the inner Jacobian is the ODE-side W by construction, so refreshing beyond iter 1 remains pointless there.Verification (local)
nlstep_tests.jlfrom 17 pass / 2 fail into 19 pass / 0 fail, with orders 1.2576 and 1.3035 back inside their bounds.newton_tests.jl+linear_nonlinear_tests.jlpass against this branch.Related findings from the same investigation (separate issues)
ndzon a rejected TR step → constant 4.7e-2 error across dts (not fixed by this PR).reinit!resets the trust radius to its build-time value.🤖 Generated with Claude Code