Repository navigation
Correct the d_discontinuities convention in the solve docstring - #4923
singhharsh1708 wants to merge 6 commits into
Conversation
Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): MERGE, risk low (L1). Reviewed head: Full reviewVERDICT: MERGE Risklow (L1: Only the Risk assessment
Blocking findingsNone. Non-blocking findingsNone. What I ranRead the supplied PR metadata first, verified the live PR remains open at the specified head, and inspected the linked issue after establishing the risk rating. Git commands, all exit 0: git rev-parse HEAD
git merge-base origin/master HEAD
git diff origin/master...HEAD
git diff --check origin/master...HEAD
git diff --name-status origin/master...HEAD
git log -1 --no-merges --format=%B
git status --shortOutput confirmed the specified head and base, one modified docstring file, no whitespace errors, and a clean worktree. The newest non-merge commit contains no agent trailers. Read-only GitHub commands, both exit 0: gh pr view 4923 --repo SciML/OrdinaryDiffEq.jl --json state,headRefOid,isDraft
gh issue view 4908 --repo SciML/OrdinaryDiffEq.jl --json number,title,body,stateRan this reviewer-written analytic probe from the head worktree: ../../jrun --project=../../tmp/SciML__OrdinaryDiffEq.jl__4923/head-env ../../tmp/SciML__OrdinaryDiffEq.jl__4923/probe.jl ../../tmp/SciML__OrdinaryDiffEq.jl__4923/head-envRan the same probe from the supplied base worktree: ../../jrun --project=../../tmp/SciML__OrdinaryDiffEq.jl__4923/base-env ../../tmp/SciML__OrdinaryDiffEq.jl__4923/probe.jl ../../tmp/SciML__OrdinaryDiffEq.jl__4923/base-envBoth exited 0, using Julia 1.13.1 through the required wrapper. Each isolated environment loaded its worktree’s DiffEqBase, OrdinaryDiffEqCore, and OrdinaryDiffEqTsit5. The probe integrates a piecewise-constant derivative across Starting-time shifts also passed in both directions. This confirms the documented endpoint hazard and the Inspected all supplied failed-job excerpts:
Compared against master’s CI, integration, and sublibrary logs. The unmatched ModelingToolkit failure remains unexplained; the diff provides no evidence connecting it to this docstring edit. The head documentation build passed. What I did not verifyDid not reproduce the issue’s full driven-mode experiment, adaptive rejection counts, KenCarp4, or AitkenNeville results. Did not rerun downstream, GPU, AD, MPI, or package-wide suites. No such executable paths change here, so G1–G3 introduce no additional verification requirement. AUTHOR_GRADE: {"task":"docs","score":5,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Focused documentation correction supported by implementation inspection and analytic probes passing on both head and base. Requires understanding endpoint stages and reversed integration direction, but changes no runtime behavior."} 🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers) |
PR: #4923 Reviewed head: a2aebc8 Risk assessment
Review reportVERDICT: CHANGES Risklow (L1: solver-keyword docstring only; executable code, tests, dependencies and public signatures are unchanged). Blocking findings
Non-blocking findings
What I ran
What I did not verify
AUTHOR_GRADE: {"task":"docs","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The strict-comparison explanation agrees with the tested solver behavior and all 65 affected-suite assertions pass. Four analytic probes disprove the unconditional shifted-stop workaround for RK4 and Tsit5 in both directions; the backward starting-time wording also remains inaccurate."} Head link: a2aebc8 🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol |
The
d_discontinuitiesentry of thesolvedocstring now calls the convention left-continuous (fatt_dis the old regime), gives the strict testt > t_dforward andt < t_dbackward, and no longer sayst >= t_dworks.With
t >= t_d, on the driven mode from the issue (40 switches in 2 ns),Tsit5rejects 914 steps and is still 5.0e-6 off at its steps wheret > t_dgives 2.7e-11, andKenCarp4takes 41 steps without a rejection and is 2.0e-2 off against 1.1e-8.The strict form is not exact either: the clamped step evaluates its end stages at
t + (t_d - t), which can round pastt_d(t_d = 0.3fromt = -10.0gives0.3000000000000007), and aprevfloat/nextfloatshift has the same problem. Fixed-step RK4 and Tsit5 then end at 11.4167 and 33.64 against an exact 9.7 (dt = 15) with retcode Success, and withdt = 25each is wrong for 127 of 500 evenly spacedt_din [-9, 9]; adaptive solves (Tsit5, RK4, KenCarp4, TRBDF2) recover. The docstring says this and recommends flipping a parameter from aDiscreteCallbackwithtstops = [t_d]when the switch must be exact (0 of 500 wrong). It also fixes the starting-time sentence: the first step begins atnextfloat(tspan[1])forward andprevfloat(tspan[1])backward.The same sentences are on the DiffEqDocs "Common Solver Options" page and SciMLBase's
Common_Keywordspage; this changes only the docstring. Making the clamped step land exactly ont_dwould remove the rounding for every method and belongs in a separate src PR.Fixes #4908.
AI Disclosure: Claude assisted with this change.