Repository navigation
Accept a vector abstol with a scalar reltol in in-place FIRK solvers - #4920
singhharsh1708 wants to merge 5 commits into
Conversation
Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): CHANGES, risk medium-low (ML2). Reviewed head: Full reviewVERDICT: CHANGES Riskmedium-low (ML2, ML8: changes non-default FIRK tolerance initialization and mutable tolerance caches; AdaptiveRadau cache coherence remains defective.) Risk assessment
Blocking findings
Non-blocking findingsNone. What I ranAbsolute locations are abbreviated below as
Source files remain unchanged in both worktrees; only the copied test file is modified on base. What I did not verify
AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":3,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The analytic regression fails on base and passes on head, and five scalar trajectories match base. Substantive repair remains necessary because AdaptiveRadau mixes tolerance generations and is omitted from the equivalence regression; coordinating stage-dependent caches across five solvers makes this moderately difficult."} 🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers) |
1fc5c1a to
68d1a48
Compare
|
The FIRK Core failure here ( With #4935 merged on top the testset passes. The two PRs also touch the same line (both read |
PR: #4920 Reviewed head: 68d1a48 Risk assessment
Review reportVERDICT: CHANGES Riskhigh (H1: AdaptiveRadau rejects a previously successful public solve input: an empty in-place state). Blocking findings
[P1] Vector reltol can produce a completely wrong solution with Success — using OrdinaryDiffEqFIRK, LinearSolve
f_small!(du, u, p, t) = (du[1] = 1e-10; nothing)
prob = ODEProblem(f_small!, [0.0], (0.0, 1.0))
sol = solve(prob, RadauIIA3(linsolve = KrylovJL_GMRES());
reltol = [1e-13], abstol = 1e-15)The head returns The new empty-keyword branch leaves both linear tolerances at [P2] Empty-state AdaptiveRadau solves now throw — using OrdinaryDiffEqFIRK
f_empty!(du, u, p, t) = nothing
solve(ODEProblem(f_empty!, Float64[], (0.0, 1.0)), AdaptiveRadau();
dt = 0.1, adaptive = false)This succeeds at the merge base. The head throws Non-blocking findingsNone. What I ranAI review by Codex (version unknown), model gpt-6.1-sol. Conversation: local session 01a1d256-24df-7002-9dd0-6d875daa44f3 (no session URL). No PR body, comments, reviews, or author rating were accessed. Here
What I did not verifyThe supplied failing CI jobs were not reproduced in their CI environments, and their failures remain unclassified. Minimum dependency versions, other Julia versions, QA, concurrent execution beyond one Julia thread, GPU, Enzyme and Zygote paths were not verified. No project-locked interpreter was available; Julia verification used the scratch manifests described above. AUTHOR_GRADE: {"task": "bugfix", "score": 2, "difficulty": 3, "rounds": 1, "author_harness": "unknown", "author_model": "unknown", "note": "Core passes 357/357 and the vector-abstol test fails at the merge base, but probes confirm a successful zero solution for small constant forcing with vector reltol and GMRES, plus a new empty-state BoundsError."} Head link: 68d1a48 🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol |
In-place
RadauIIA3,RadauIIA5,RadauIIA9,AdaptiveRadauandGaussLegendrenow solve with a vectorabstoland a scalarreltolinstead of throwing aMethodErrorat init, and in-placeRadauIIA3also takes a vectorreltol.The in-place caches in
firk_caches.jltypedatolfromreltol, so a scalarreltolgave a scalar field that was then assigned a vector;atolis now an array filled in place.RadauIIA3passesminimum(reltol)to the linear solver whenreltolis a vector (asnewton_tolerancedoes), where master threw aMethodError.AdaptiveRadaureadsrtolandatolfrom the cache after updating them, which keeps empty states working with the arrayatol.This changes scalar-tolerance
AdaptiveRadauresults on purpose: master's first step used the rawreltolfor both tolerances, so on this branch Van der Pol (mu = 100, u0 = [2, 0], tspan (0, 2), reltol 1e-6, abstol 1e-7) takes 91 steps instead of 92. The other four methods take the same number of steps as master on that problem, and an equal-valued vectorabstolgives the same solve as the scalar one for all five methods. The vector-tolerance and Krylov tests fail without the src change; the empty-state test guards the arrayatol.Depends on #4935: the changed
AdaptiveRadaustep pattern exposes the dense-output bug #4925 in the sparse-vs-dense test atode_firk_tests.jl:79(1.33e-4 against an atol of 1e-4), which is why FIRK Core and downgrade are red here. With #4935 stacked the difference is below 1e-6. #4935 also bumps FIRK to 2.8.10, so this PR moves to 2.8.11 once it merges.Not covered here:
RadauIIA5,RadauIIA9,AdaptiveRadauandGaussLegendrenever set the linear solver'sabstol(only a scalarreltolper solve), so a Krylov solve can stop at LinearSolve's default and return a wrong answer with scalar tolerances too (RadauIIA5with GMRES gives 0.0 instead of 1e-10; master as well).Fixes #4911.
AI Disclosure: Claude assisted with this change.