Skip to content

Store AdaptiveRadau dense output at the max-stage nodes - #4935

Open
singhharsh1708 wants to merge 3 commits into
SciML:masterfrom
singhharsh1708:adaptiveradau-dense-stage-count
Open

singhharsh1708 wants to merge 3 commits into
SciML:masterfrom
singhharsh1708:adaptiveradau-dense-stage-count

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

AdaptiveRadau now stores each accepted step's collocation polynomial at the nodes of its largest tableau, so the interpolant no longer depends on the cache's current stage count. It read num_stages and index from the cache, which after the solve hold the last step's values, so every step taken with a different stage count was interpolated with the wrong nodes.
A polynomial of degree s is reproduced exactly from its values at the max-stage nodes, so the interpolant always uses tabs[end], and the predictor reads the previous step's stages back at the current nodes. With the stage count pinned (min_order == max_order) the weights are exactly 0 and 1, and solutions are bitwise identical to master.
On the issue's problem (u' = cos(t) u, reltol 1e-8), the dense error goes from 0.216 to 1.4e-6 in place and from 0.209 to 7.3e-7 out of place, with 22 and 21 steps against master's 23 and 22. With a variable stage count the predictor now sees the previous polynomial at the right nodes after a stage-count change, so step sequences differ slightly: on ROBER, OREGO and stiff Van der Pol at 1e-6, 1e-8 and 1e-10, accepted steps change by -65 to +2 and f evaluations by -10% to +11% (stiff Van der Pol at 1e-10 goes from 5 to 14 rejections), and end and dense errors are at most 1.9 times master's or better (OREGO 1e-10 dense 5.6e-8 to 1.2e-9).
In place, perform_step! also read rtol and atol before updating them for the step's stage count, so a step after a change used the previous count's tolerances, while out of place recomputes them every step. It now reads them after the update, so equal-valued scalar and vector tolerances give the same solve (Van der Pol with mu = 100: 92 and 91 steps before, 91 for both now).
The new testset requires the dense error below 1e-5 in place and out of place, and fewer than 40 accepted steps, which fails if the predictor is left reading the old nodes (63 and 58 steps); it fails on master. The Newton-tolerance testset now also checks the scalar and vector tolerance solves are equal, which fails without the read change. FIRK Core, QA and the root Integrators_I group pass.
This rewrites the AdaptiveRadau stage-storage block that #4924 takes out of its extrapolant != :constant guard; whichever lands second keeps the other's change (no guard, max-node storage).
Fixes #4925
AI Disclosure: Claude assisted with this change.

@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): MERGE-WITH-ISSUES, risk medium (M3). Reviewed head: efb389d284b8dc83d36dffacb4f35c7afdf131bd.

Full review

VERDICT: MERGE-WITH-ISSUES
PR: #4935
Round: 1
Head: efb389d
Base: 45ff46a
Slot: 1
Reviewer: Codex CLI 0.163.0 / gpt-6.1-sol
Session: 01a19774-743e-7c93-8d8a-14be1f9bbd65
Reviewed: 2026-11-01T14:25:39Z
Summary: The PR fixes AdaptiveRadau dense output across stage-count changes and refreshes Newton tolerances before use. The regressions fail on base and pass on head; the remaining findings concern avoidable performance overhead and an inaccurate description claim.

Risk

medium (M3: repeated node conversions measurably slow fixed-order configurations of this non-default solver.)

Risk assessment

  • Risk: medium (M3: measured peripheral-path slowdown from additional polynomial conversions).
  • Blast radius: AdaptiveRadau’s in-place and out-of-place stage storage, predictor, dense interpolation, and in-place tolerance handling.
  • Evidence: The changed test file passes on head and fails on base against an analytic solution. Additional interpolation and tolerance probes confirm the fixes. FIRK CI passes; four failed-job signatures match master, while the ModelingToolkit failure remains unclassified.
  • Verdict: MERGE-WITH-ISSUES
  • Round: 1
  • Blocking priorities: none
  • Merge: merge

Blocking findings

None.

Non-blocking findings

  1. [P3] Avoid identity resampling - confirmed by running. At lib/OrdinaryDiffEqFIRK/src/firk_perform_step.jl:2008 and lib/OrdinaryDiffEqFIRK/src/firk_perform_step.jl:2101, full conversions execute even when the current and maximum tableau nodes coincide. For a 16-component linear problem with fixed order 25 and 100 fixed steps, the out-of-place benchmark increased from 3.551 ms and 24,666,240 allocated bytes on base to 4.355 ms and 31,123,392 bytes on head: approximately 23% more time and 26% more allocation. The endpoint was identical. Add a direct-copy path when the node sets coincide; benchmark figures are indicative minima from five warmed samples.

  2. [P3] Qualify the pinned-order bitwise-identity claim - confirmed by running. The tolerance read at lib/OrdinaryDiffEqFIRK/src/firk_perform_step.jl:2064 changes adaptive stepping even without stage-count changes. For the analytic in-place problem with min_order = max_order = 13, base accepted eight steps and head accepted seven, contradicting the PR description’s blanket claim of bitwise-identical pinned-order solutions. Both remain accurate. Qualify the claim as applying to the resampling operation and describe the tolerance-refresh effect separately.

What I ran

Julia 1.13.1, exclusively through the supplied jrun wrapper. Wrapper and probe paths below are expressed relative to each worktree.

  • Git inspection, exit 0:

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

    Head and merge-base matched the supplied SHAs. No author harness/model trailers were present. The head worktree remained clean; the base worktree contained only the copied test-file change. No source files were changed.

  • Linked issue inspection, exit 0:

    gh issue view 4925 --repo SciML/OrdinaryDiffEq.jl --json title,body,url,state
  • Copied head’s lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl into the corresponding location in the supplied base worktree, then ran this command in each worktree:

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

    Head: exit 0. Relevant output tail:

    AdaptiveRadau dense output across stage-count changes | 4/4 pass
    AdaptiveRadau Newton tolerance follows the stage count | 9/9 pass
    FIRK stiff member of an AutoSwitch composite (#4364)   | 10/10 pass
    

    Base: exit 1. Relevant output tail:

    Evaluated: 0.21634748188421105 < 1.0e-5
    Evaluated: 0.2085769144500973 < 1.0e-5
    AdaptiveRadau dense output across stage-count changes | 2 passed, 2 failed
    

    The base failure stopped execution before the following tolerance testset.

  • Ran the same short probe in each worktree:

    ../../jrun --project=lib/OrdinaryDiffEqFIRK ../../tmp/SciML__OrdinaryDiffEq.jl__4935/probe.jl

    It checked buffered interpolation against exp(sin(t)), several order ranges, scalar/vector tolerance equality, and warmed fixed-order performance.

    Head: exit 0:

    scalar_vector_equal=true steps=91/91
    additional dense-output forms | 8 passed
    

    Maximum buffered-interpolation error was approximately 1.14e-6.

    Base: exit 1:

    scalar_vector_equal=false steps=91/90
    additional dense-output forms | 4 passed, 4 failed
    

    The four failures reproduced incorrect interpolation across stage-count changes.

  • Inspected the supplied CI checks and failed-log excerpts:

What I did not verify

  • Did not rerun package-wide, downstream, GPU, external AD, Reactant, or MPI suites locally.
  • Did not reproduce or establish the cause of the ModelingToolkit CI failure.
  • Did not independently repeat the description’s complete ROBER/OREGO/Van der Pol benchmark matrix.
  • Performance measurements cover one small fixed-order workload, not a broad benchmark suite.

AUTHOR_GRADE: {"task":"bugfix","score":4,"difficulty":3,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Analytic interpolation regressions fail on base and pass on head, and the tolerance equality fix is independently confirmed. The polynomial/cache change is a moderate solver edge case; avoidable identity-resampling overhead and the pinned-order bitwise-identity claim need minor cleanup."}


🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers)
Review: unreviewed
Session: 01a19774-743e-7c93-8d8a-14be1f9bbd65

@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

PR: #4935

Reviewed head: efb389d
Raw SHA-256: 6c1ede2500d51f99c077f73e1307d68c708b41b16d702100e3deddd60584bc68
Source SHA-256: a87fb2de9ce264f8338beb2986c665fb20882267dd3f5d208cf157113d251f0b
Source: standard-review-v1 (harness-log)
Reviewer session: 01a1d256-e57f-7c92-b6f3-5ce7ef6b6578
Slot: 1
Confidence: unknown
Own risk fixed before author rating/discussion: yes

Risk assessment

  • Risk: medium (M3)
  • Verdict: CHANGES
  • Reviewed head: efb389d

Review report

VERDICT: CHANGES
Round: 1
Head: efb389d
Slot: 1
CI: CI is not green (regress:SciMLSensitivity.jl/Core4/1 / Downstream Tests - Core4; ModelingToolkit.jl/InterfaceI/1 / Downstream Tests - InterfaceI)
Session: 01a1d256-e57f-7c92-b6f3-5ce7ef6b6578

Risk

medium (M3: AdaptiveRadau's step hot path adds repeated polynomial projections; their performance and numerical behavior could not be measured).

Blocking findings

  • [P2] Needs verification of the changed numerical paths — lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl:303. The affected Core suite, new regression tests, and adversarial probes could not execute within the supplied host limits. A 3600-second admission wait returned exit code 124: all 120 samples failed at least one required condition (available memory ≥16 GiB, load <32, fewer than five existing Julia worker groups). No Julia test process started. This is an execution gap, not a confirmed functional defect. Run Core and the prepared probes, including failing-before/passing-after controls, before accepting this review. Missing runtime evidence remains blocking regardless of P2 deferral.

  • Verdict: CHANGES

  • Round: 1

  • Blocking priorities: P2

Non-blocking findings

  • lib/OrdinaryDiffEqFIRK/src/firk_interpolants.jl:143, lib/OrdinaryDiffEqFIRK/src/firk_perform_step.jl:2099, and lib/OrdinaryDiffEqFIRK/src/firk_perform_step.jl:2364: Cost concern from reading; no measured slowdown. Grid-only Lagrange weights are rebuilt inside each step, with additional weighted array passes. This also happens when the current and maximum grids coincide and the projection is the identity. Consider caching projection matrices or retaining a direct-copy case, and measure fixed-order and variable-order solves.

What I ran

  • Blind diff/source inspection with git diff origin/master...HEAD, including affected initialization, caches, controllers, interpolation, and test dispatch. No PR discussion or author risk rating was read.

  • git rev-parse HEAD returned the requested full SHA. git diff --check origin/master...HEAD and final git status --porcelain produced no output. No checkout edits were made.

  • julia --startup-file=no --version returned julia version 1.13.2.

  • uv run --locked python --version returned Python 3.15.0, with the warning that --locked has no effect outside a project. No uv.lock, root Julia Manifest, or FIRK Julia Manifest was present; no locked project environment was available.

  • Exported the requested TMPDIR and preserved the supplied JULIA_DEPOT_PATH. Prepared a scratch environment using checkout packages and FIRK's declared test dependencies.

  • Attempted the following resource-gated pipeline, where REVIEW_SCRATCH denotes the requested scratch directory:

    timeout 3600 bash "$REVIEW_SCRATCH/await_resources.sh" \
      systemd-run --user --scope --unit=review-4935-setup \
      -p MemoryMax=8G -p MemoryHigh=infinity \
      env JULIA_NUM_THREADS=1 JULIA_NUM_PRECOMPILE_TASKS=4 \
      JULIA_PKG_PRECOMPILE_AUTO=0 OPENBLAS_NUM_THREADS=1 \
      julia --startup-file=no --project="$REVIEW_SCRATCH/env" \
      "$REVIEW_SCRATCH/setup.jl"

    Observed: exit 124; 120 admission samples; zero admitted samples; zero pipeline stages started. The scope was subsequently confirmed inactive/not found. All commands started by this review have finished.

  • AI reviewer: codex-cli 0.164.0, model gpt-6.1-sol; local session 01a1d256-e57f-7c92-b6f3-5ce7ef6b6578 (no session URL).

What I did not verify

  • FIRK GROUP=Core / Pkg.test("OrdinaryDiffEqFIRK"; allow_reresolve=false): queued, never executed; dependency instantiation also never started.
  • Prepared but unrun probes: analytic Float64/Float32 vector accuracy, scalar states, both interpolation APIs, actual stage increases and forced decreases, polynomial reproduction, continuous-event timing, and scalar/vector tolerance parity.
  • Prepared but unrun base controls using the changed source files from merge base 45ff46afe9919acf32e9af8058705073e0995503; no failing-before/passing-after result is claimed. Fixed-step performance comparisons were also unrun.
  • QA, formatter, docs, and the two reported downstream CI failures; their causes and default-branch status remain unverified.

AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":3,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The diff adds independent analytic dense-output coverage and scalar/vector tolerance parity without weakening existing tests. Runtime correctness, regression reproduction, and performance remain unverified because the one-hour resource admission wait never allowed execution."}

Head link: efb389d


🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol
Conversation: local session 01a1d256-e57f-7c92-b6f3-5ce7ef6b6578 (no session URL)

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.

AdaptiveRadau dense output is wrong on steps taken before a change in stage count

2 participants