Skip to content

NSA: gate iter-1 convergence on inner solver retcode (#3817)#3893

Open
singhharsh1708 wants to merge 1 commit into
SciML:masterfrom
singhharsh1708:nsa-trustregion-ndz-convergence-3817
Open

NSA: gate iter-1 convergence on inner solver retcode (#3817)#3893
singhharsh1708 wants to merge 1 commit into
SciML:masterfrom
singhharsh1708:nsa-trustregion-ndz-convergence-3817

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #3817. Standalone on master.

Bug

nlsolve! has an iteration-1 shortcut:

if (iter == 1 && ndz < 1.0e-5) || ...

ndz is the displacement of the iterate. For a plain (modified-)Newton inner solver a tiny first-iterate displacement means the initial guess was already the solution, so the shortcut is sound. For a globalized inner solver it is not: a TrustRegion step that gets rejected or truncated leaves the iterate essentially unmoved, which produces the same tiny ndz while the solver has not actually decided anything. The outer loop reads that as convergence and accepts the stage with no correction applied.

Reproduced on current master, Robertson with an analytic jac, FBDF + NonlinearSolveAlg(TrustRegion()), fixed dt = 1.0:

retcode: Success
naccept: 1000
u[end]:  [1.0, 0.0, 0.0]     # identical to u0 — never moved
njacs:   2
nnonliniter: 1000            # exactly one inner iteration per step

Every step "converges" instantly, the state never changes, and the solve reports Success. NonlinearSolveAlg(NewtonRaphson()) on the same problem correctly reports ConvergenceFailure.

Fix

Only take the iter-1 shortcut when the inner cache's own retcode says it actually converged:

if (iter == 1 && ndz < 1.0e-5 && _nsa_inner_converged(nlsolver)) || ...

Iterations past the first derive η/θ from real step history and are not vulnerable to the false positive, so they are untouched — an earlier attempt that also gated the η * ndz < κ branch cost roughly one extra outer iteration per solve and pushed marginal solves over the default max_iter = 10, so the gate is deliberately confined to iter == 1.

The predicate excludes AbstractSimpleNonlinearSolveAlgorithm inner algs. Those caches are solved to completion rather than stepped and never update their own retcode, so without the exclusion they would lose the shortcut and run two full inner solves per stage.

Result

TrustRegion now reaches the same verdict as NewtonRaphson everywhere tested — no more silent Success on a frozen state:

dt TrustRegion (before) TrustRegion (after) NewtonRaphson
0.01 Success, u frozen at u0 ConvergenceFailure ConvergenceFailure
0.1 Success, u frozen at u0 ConvergenceFailure ConvergenceFailure
1.0 Success, u frozen at u0 ConvergenceFailure ConvergenceFailure
10.0 Success, u frozen at u0 ConvergenceFailure ConvergenceFailure

Where the solves genuinely do converge (dt = 2^-15 … 2^-18), results are bit-identical to before the change, with no extra f-evals.

Tests

Two testsets added to nsa_jacobian_reuse_tests.jl: one asserting a frozen state is never reported as a successful solve and that TrustRegion agrees with NewtonRaphson on the verdict, one asserting TrustRegion still converges to the reference where it should. Both fail on master and pass with this change.

The OrdinaryDiffEqNonlinearSolve suite passes, as does OrdinaryDiffEqMultirate (88/88), which shares nlsolve! for its implicit MRI-GARK stages.

AI Disclosure

Claude assisted with this work.

@ChrisRackauckas

Copy link
Copy Markdown
Member

don't stack this

@singhharsh1708
singhharsh1708 force-pushed the nsa-trustregion-ndz-convergence-3817 branch from 4f63a59 to eae4139 Compare July 13, 2026 17:30
@singhharsh1708

Copy link
Copy Markdown
Contributor Author

don't stack this

done.

@singhharsh1708
singhharsh1708 force-pushed the nsa-trustregion-ndz-convergence-3817 branch 3 times, most recently from adbd943 to 8628e6d Compare July 17, 2026 02:10
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.

NonlinearSolveAlg(TrustRegion) + W-reuse: displacement-based ndz declares convergence on rejected TR steps → silently wrong solutions

2 participants