Skip to content

Give FIRK smooth_est = false an unfiltered error estimate - #4930

Open
singhharsh1708 wants to merge 3 commits into
SciML:masterfrom
singhharsh1708:firk-unsmoothed-estimate
Open

singhharsh1708 wants to merge 3 commits into
SciML:masterfrom
singhharsh1708:firk-unsmoothed-estimate

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

With smooth_est = false and an identity mass matrix, RadauIIA5, RadauIIA9 and AdaptiveRadau now estimate the error as -(dt/γ) (f₀ + Σ eᵢzᵢ/dt), the limit of the smoothed estimate W1⁻¹ (f₀ + M Σ eᵢzᵢ/dt) for small dt J, with W1 = J - (γ/dt) M. With any other mass matrix that limit needs M⁻¹, which a singular M does not have, so the smoothed estimate is used; using the identity formula there lost error control (maximum error 5.3e-3 at tolerance 1e-6 with M = 1e-4 I).
Before, the flag only skipped the W1 solve: in place utilde stayed the transformed stage w2, and out of place it stayed f₀ + M Σ eᵢzᵢ/dt, a rate about γ/dt times too large. The same applies in the first-step retry branch, where the estimate is added to uprev.
On the pulse problem from the issue, master takes 88,779 to 182,216 steps or stops (MaxIters, Unstable) in every smooth_est = false case; with this change all twelve finish in 32 to 175 steps, within one step of the default, with relative errors between 7.3e-14 and 1.7e-8. Runs with smooth_est = true are identical to master.
The new testset (u' = -50 (u - cos t)) fails on master: in place the unsmoothed runs hit MaxIters, and out of place RadauIIA5 takes 357 steps against 47. It also checks that after one step at dt = 0.01 the unsmoothed estimate is within 2% of the smoothed one (a copy with γ dropped at all twelve sites fails it), and that with a scaled diagonal mass matrix smooth_est = false gives the smoothed solve exactly. FIRK Core passes.
The smooth_est docstring, "TBD" until now, describes both settings. RadauIIA3, which has no smooth_est keyword, now gets its own keyword list without it.
Part of #4912; the extrapolant half is #4924.
AI Disclosure: Claude assisted with this change.

@singhharsh1708 singhharsh1708 changed the title Give FIRK smooth_est = false an unfiltered error estimate in units of u Give FIRK smooth_est = false an unfiltered error estimate Oct 27, 2026
@singhharsh1708

Copy link
Copy Markdown
Member Author

🤖 Automated comment from an AI agent running as @singhharsh1708, not written or reviewed by the account holder.

Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): CHANGES, risk medium-low (ML2). Reviewed head: 1bab25d78c0d295f1acc5bd48f0860d7d4e8b306.

Full review

VERDICT: CHANGES
PR: #4930
Round: 1
Head: 1bab25d
Base: 45ff46a
Slot: 1
Reviewer: Codex CLI 0.163.0 / gpt-6.1-sol
Session: 01a1975f-a79f-7b43-94f8-f4f19a62c6ce
Reviewed: 2026-11-01T14:10:48Z
Summary: The PR corrects unsmoothed Radau error estimates and preserves filtering for non-identity mass matrices. Fail-before/pass-after is confirmed, but the committed numerical regression needs an independent accuracy oracle under G1.

Risk

medium-low (ML2: Error-estimate correction for non-default Radau configurations; default filtered estimates remain unchanged.)

Risk assessment

  • Risk: medium-low (ML2: Peripheral solver numerics, with confirmed regression reproduction and independent reviewer accuracy checks.)
  • Blast radius: RadauIIA5, RadauIIA9, and AdaptiveRadau with smooth_est=false, including first-step retries and mass-matrix handling.
  • Evidence: New testset passes 24/24 on head and fails 17/24 on base. Analytic identity-matrix probes pass for both problem forms and all three solvers. FIRK Core and QA CI pass.
  • Verdict: CHANGES
  • Round: 1
  • Blocking priorities: P2
  • Merge: needs changes (retain an independent numerical accuracy assertion in the PR).

Blocking findings

  1. [P2] Add an independent accuracy oracle for the changed estimator - suspected from reading. At lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl:324, the expected solution comes from the same solver with smoothing enabled. The remaining new checks compare step counts, error estimates, or two settings of that solver. These establish behavioral agreement but do not retain the independent numerical accuracy test required by G1.

    Keep those comparisons and add an analytic assertion for both problem forms and all three algorithms. The existing equation has the exact solution
    50 * (50*cos(t) + sin(t))/2501 .+ (u0 .- 2500/2501)*exp(-50*t).
    Reviewer probes support the correction, but those external probes do not become a regression guard in the PR’s CI.

Non-blocking findings

None.

What I ran

Julia 1.13.1, exclusively through the prescribed wrapper. Commands below use worktree-relative executable and probe paths.

Git inspection commands exited 0:

git rev-parse HEAD
git merge-base origin/master HEAD
git diff origin/master...HEAD
git log -1 --format=%B
git status --short
git diff --check

HEAD and merge-base matched the specified SHAs. The latest commit has no Agent-Harness or Agent-Model trailers. The head worktree remains clean.

Read linked issue #4912 with:

gh issue view 4912 --repo SciML/OrdinaryDiffEq.jl --json number,title,body,url

Exit 0.

Ran the changed test file on head:

../../jrun --project=lib/OrdinaryDiffEqFIRK -e 'using TestEnv; TestEnv.activate(); using Test, SafeTestsets; println("Julia ", VERSION); include("lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl")'

Exit 0. Output tail:

Unsmoothed error estimate (smooth_est = false) | 24  24
FIRK stiff member of an AutoSwitch composite (#4364) | 10  10

Copied the PR’s changed lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl unchanged into the supplied merge-base worktree and ran the same command there. Exit 1:

Unsmoothed error estimate (smooth_est = false) | 7 passed, 17 failed
ERROR: LoadError: Some tests did not pass: 7 passed, 17 failed, 0 errored, 0 broken.

Failures reproduced MaxIters, excessive step counts (995 < 92 and 357 < 94), incorrect estimates, and differing mass-matrix solutions. Only the authorized test-file copy changed in the base worktree.

Ran an independent analytic probe on head:

../../jrun --project=lib/OrdinaryDiffEqFIRK -e 'using TestEnv; TestEnv.activate(); include("../../tmp/SciML__OrdinaryDiffEq.jl__4930/analytic_probe.jl")'

Exit 1:

Independent analytic accuracy | 30 passed, 2 failed, 1 errored

All six identity-matrix cases succeeded, with maximum endpoint error 8.44e-9. Initial dt=0.1 produced rejected steps, exercising first-step retry handling. Two singular-matrix RadauIIA5 cases exceeded the exploratory 1e-6 bound with error 1.1241585559096379e-6. Out-of-place AdaptiveRadau with a diagonal mass matrix threw DimensionMismatch at the unchanged mass_matrix * w expression.

Used the second short probe on base to attribute these failures:

../../jrun --project=lib/OrdinaryDiffEqFIRK -e 'using TestEnv; TestEnv.activate(); include("../../tmp/SciML__OrdinaryDiffEq.jl__4930/baseline_probe.jl")'

Exit 0:

base default RadauIIA5 singular iip final_error=1.1241585559096379e-6
base default RadauIIA5 singular oop final_error=1.1241585559096379e-6
base AdaptiveRadau oop scaled smooth_est=true exception=DimensionMismatch
base AdaptiveRadau oop scaled smooth_est=false exception=DimensionMismatch
Baseline attribution | 4 passed, 4 total

Thus neither exploratory failure is attributed to this PR.

Inspected supplied FIRK CI results, PR CI failures, and downstream failures. Compared against recent master CI and master downstream logs: the ReverseDiff tape errors, stiff-adjoint unexpected passes, ensemble assertion, and GPUCompiler warning failure have matching signatures.

What I did not verify

  • No package-wide, downstream, GPU, or parameter-AD suites were run locally.
  • The linked issue’s nanosecond pulse example was not rerun.
  • The ModelingToolkit components.jl:446 equation-orientation CI failure has no matching signature in the supplied master logs; its cause remains unattributed.
  • The analytic probe aborted before the final out-of-place AdaptiveRadau singular-matrix case.

AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":3,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The regression fails on base and passes on head, and independent reviewer probes support the identity-matrix correction. The PR still needs a retained independent accuracy oracle for its numerical change; repairing solver error estimation is a moderate numerical task."}


🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers)
Review: unreviewed
Session: 01a1975f-a79f-7b43-94f8-f4f19a62c6ce

@singhharsh1708
singhharsh1708 force-pushed the firk-unsmoothed-estimate branch from 1bab25d to 7a50a3a Compare November 1, 2026 15:55

This branch has not been deployed

No deployments
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.

1 participant