Repository navigation
Store FIRK stages for dense output with extrapolant = :constant - #4924
singhharsh1708 wants to merge 2 commits into
Conversation
Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): MERGE, risk medium-high (MH1). Reviewed head: Full reviewVERDICT: MERGE Riskmedium-high (MH1: changes interpolation results, an OrdinaryDiffEq main lane, for the constant FIRK predictor.) Risk assessment
Blocking findingsNone. Non-blocking findingsNone. What I ranCommands below abbreviate the supplied Julia runner as Read Read-only verification commands, all exit 0: git rev-parse HEAD
git rev-parse origin/master
git merge-base origin/master HEAD
git diff origin/master...HEAD
git diff --check origin/master...HEAD
git log -1 --no-merges --format=%B
gh pr view 4924 --repo SciML/OrdinaryDiffEq.jl --json number,url,state,isDraft,headRefOid,baseRefName,baseRefOid
gh issue view 4912 --repo SciML/OrdinaryDiffEq.jl --json number,title,body,state,urlHEAD matched the supplied SHA; live PR state was Copied the complete changed test file into base, without modifying source files: TEST=lib/OrdinaryDiffEqFIRK/test/ode_firk_tests.jl
cp "$TEST" "$BASE/$TEST"
cmp "$TEST" "$BASE/$TEST"Both commands exited 0. Ran this identical command once from head and once from base, through the supplied runner using Julia 1.13.1, four Julia threads and one OpenBLAS thread: "$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, output tail: Base exit 1, output tail: The eight Radau failures at Supplementary commands: # From head:
"$JRUN" --project=lib/OrdinaryDiffEqFIRK "$PROBES/probe.jl" "$PROBES/head-endpoints.txt"
# From base:
"$JRUN" --project=lib/OrdinaryDiffEqFIRK "$PROBES/probe-base.jl" "$PROBES/base-endpoints.txt"
cmp "$PROBES/head-endpoints.txt" "$PROBES/base-endpoints.txt"The probes cover all five algorithms, three state/function forms, both predictors and fixed/adaptive stepping, checking retained
Final head working tree was clean. Base contained only the copied test-file change. Inspected supplied CI excerpts and check URLs:
What I did not verifyNo package-wide or heavy downstream suites were rerun. GPU hardware and outer AD/sensitivity paths were not exercised locally; no GPU kernels or AD rules changed. Variable-order AdaptiveRadau retained history, callbacks and The supplementary head probe’s return-code assertions were not successfully executed because of my missing qualification; its accuracy and end-time assertions passed. The unmatched ModelingToolkit CI failure was not reproduced locally. AUTHOR_GRADE: {"task":"bugfix","score":5,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Focused stage-storage fix with an independent analytic regression: eight cases fail on base and all ten pass on head. Endpoint histories and step counts match across 60 supplementary configurations; the task is a small local solver-cache correction."} 🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers) |
PR: #4924 Reviewed head: fe0328d Risk assessment
Review reportVERDICT: CHANGES Riskmedium-low (ML2: numerical fix confined to non-default FIRK extrapolation; independent analytic regression fails before and passes after; ten tested default-path fingerprints are unchanged) Blocking findings
Non-blocking findingsNo introduced code defect was confirmed. The probes exposed these pre-existing problems; they should be handled separately:
What I ranAI reviewer: codex-cli 0.164.0, model gpt-6.1-sol; actual local session ID above, no session URL available. All Julia tests preserved the supplied layered
What I did not verifyThe supplied downstream regressions and their CI logs; QA/formatter/docs checks; other Julia versions or platforms. No PR body, comments, reviews, earlier same-head opinions or author risk rating were consulted. No GitHub writes were made. AUTHOR_GRADE: {"task":"bugfix","score":4,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Independent analytic regression fails 8/10 before and passes 10/10 after; all 337 FIRK Core assertions pass and ten default-path fingerprints match. Additional counterexamples predate the diff; supplied downstream regressions remain unverified."} Head link: fe0328d 🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol |
|
Both downstream failures are green on master and these runs predate that, so I merged master to re-run. The GaussLegendre matrix-state DimensionMismatch is on master too and gets its own issue; the AdaptiveRadau dense-output part is #4935. |
With
extrapolant = :constant, the dense output ofRadauIIA3,RadauIIA5,RadauIIA9andAdaptiveRadauis the collocation polynomial again instead of the step's starting value, sosaveatandsol(t)inside a step are right.GaussLegendrenow stores its stages the same way, but its dense output is the generic Hermite interpolant, which does not read them, so its results are unchanged.The stages in
integrator.kwere stored after an accepted step only whenextrapolant != :constant. That guard dates from when they served only as the next step's predictor; the interpolants now read them too. They are now stored after every accepted step, and:constantstill only sets the next step's Newton guess to zero, as it already zeroeszandkat the start of each step, so the steps and their values are unchanged.On
u' = cos(t) uover (0, 4) at reltol 1e-8 withsaveat = 0.01, the:constanterror of the four Radau methods goes from 2.0e-2 to 1.2 on master to 2.8e-8 to 1.8e-6, in place and out of place, with the same step counts; forRadauIIA3,RadauIIA5andRadauIIA9that is about what the default extrapolant gives.AdaptiveRadau's default dense output is off by 0.2 on this problem for a separate reason: its interpolant uses the current stage count for every past step (#4925, fixed in #4935). The new testset fails on master for the four Radau methods (GaussLegendrewas not affected), and FIRK Core and QA pass on Julia 1.12.This is the
extrapolanthalf of #4912;smooth_est = falseneeds its own change. Against #4935, keep its unguarded storage at the max-stage nodes. FIRK goes to 2.8.10, the same version as #4839 and #4920.AI Disclosure: Claude assisted with this change.