Repository navigation
Run PRK and PDIRK stages serially inside another threaded region - #4940
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 (ML8). Reviewed head: Full reviewVERDICT: CHANGES Riskmedium-low (ML8: changes a shared threading helper; targeted runs establish preserved results and reproduce the original exception.) Risk assessment
Blocking findings
Non-blocking findingsNone. What I ranCommands below substitute
What I did not verify
AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The small threading fix passes targeted PRK and PDIRK checks, and the copied regression fails on base. Substantive repairs remain because the regression is disconnected from CI and the implementation relies on a non-public runtime symbol."} 🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers) |
09cfafa to
1c0def3
Compare
89553bb to
1c61b6f
Compare
PR: #4940 Reviewed head: 1c61b6f Risk assessment
Review reportVERDICT: CHANGES Riskmedium-low (ML8: changes shared stage-threading execution for PRK, PDIRK and optional FIRK/SDC paths; adds a nested-threading regression test, but runtime verification is outstanding). Blocking findings
This is a needs-verification verdict. No numerical regression or other code correctness failure was confirmed by running.
Non-blocking findingsNone identified from static inspection. The new equality assertion checks scheduling equivalence; no numerical formulas or existing assertions are changed. What I ranAI reviewer: codex-cli 0.164.0, model gpt-6.1-sol. The Session field gives the actual local session ID; no conversation URL is available.
What I did not verifyThe PRK functional and QA suites, existing convergence tests, and prepared targeted probes were not executed. Those probes cover both state representations, Bool/BaseThreads options, nested static/dynamic loops, exceptions, analytic fifth-order convergence, and the added regression test with the original helper restored only in memory. Dependency instantiation also remained unexecuted. I did not verify SciMLSensitivity/Core4 against the default branch, other supported Julia versions, or a locked dependency environment. Scratch preparation did not modify the checkout. No PR body, discussion, author rating or earlier review was inspected. AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The diff is focused and adds a nested-threading regression test. Runtime correctness and failing-before/passing-after evidence were not verified because resource admission failed for one hour; the supplied downstream CI regression remains unresolved."} Head link: 1c61b6f 🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol |
|
The jl_in_threaded_region point is the one answered earlier in this thread, nothing new there. Core4 has been green on master since 11-04 and that run predates it, so I merged master to re-run. |
KuttaPRK2p5andPDIRK44, which default tothreading = true, now run their stages serially when the solve is already inside a threaded region, instead of throwing "@threads :staticcannot be used concurrently or nested". OrdinaryDiffEqCore's_threaded_executegets the samejl_in_threaded_regioncheck that #4929 adds to the extrapolation methods' own helper.Called from inside
Threads.@threads(:staticor:dynamic), both methods throw on master, in place and out of place; here they run and give the sameuas the serial solve. The new PRK testset fails on master with 1 and with 4 threads and passes here. Core QA, PRK Core and PDIRK Core pass. PDIRK has no[sources], so its tests use the registered Core and are not a test of this change; I checked it with a local environment instead.OrdinaryDiffEqCore goes to 4.20.2, the same version as #4921, #4931 and #4932.
AI Disclosure: Claude assisted with this change.