Repository navigation
Store AdaptiveRadau dense output at the max-stage nodes - #4935
singhharsh1708 wants to merge 3 commits into
Conversation
Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): MERGE-WITH-ISSUES, risk medium (M3). Reviewed head: Full reviewVERDICT: MERGE-WITH-ISSUES Riskmedium (M3: repeated node conversions measurably slow fixed-order configurations of this non-default solver.) Risk assessment
Blocking findingsNone. Non-blocking findings
What I ranJulia 1.13.1, exclusively through the supplied
What I did not verify
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) |
PR: #4935 Reviewed head: efb389d Risk assessment
Review reportVERDICT: CHANGES Riskmedium (M3: AdaptiveRadau's step hot path adds repeated polynomial projections; their performance and numerical behavior could not be measured). Blocking findings
Non-blocking findings
What I ran
What I did not verify
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 |
AdaptiveRadaunow 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 readnum_stagesandindexfrom 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 readrtolandatolbefore 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 != :constantguard; whichever lands second keeps the other's change (no guard, max-node storage).Fixes #4925
AI Disclosure: Claude assisted with this change.