Skip to content

Clamp parameter_estim_ode! particle positions to the box elementwise - #140

Merged
ChrisRackauckas merged 2 commits into
SciML:mainfrom
ChrisRackauckas-Claude:ode-pso-elementwise-bounds
Oct 31, 2026
Merged

ChrisRackauckas merged 2 commits into
SciML:mainfrom
ChrisRackauckas-Claude:ode-pso-elementwise-bounds

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

🚧 UNREVIEWED — awaiting review by Chris Rackauckas. Updated by an AI agent running as Chris Rackauckas; Chris has not reviewed this follow-up.
Harness: Codex CLI 0.162.1 · Model: gpt-6.1-sol
Conversation: local session 01a14b6e-4c2a-77d2-8d02-6dcd6c6e46d4 (no session URL)

Agent follow-up

What changed and why. ode_update_particle_states! (the particle update kernel behind the internal ParallelParticleSwarms.parameter_estim_ode!) clamped positions with max(particle.position, lb) / min(update_pos, ub). On SVectors, max/min compare the whole vectors lexicographically and return one of them, so a particle could keep an out-of-box coordinate, or have an in-box coordinate overwritten by the bound. The fix broadcasts (max. / min.), which is what update_particle_states! in src/kernels.jl already does. test/ode_pso.jl checks that the kernel output equals clamp.(x, lb, ub) for three positions that each sit outside the box in at least one coordinate.

Verification (PR branch merged locally onto current main @ df60a25; the merge is clean, Julia 1.13.0):

Test failing with the src/ode_pso.jl fix reverted:

parameter_estim_ode! particle update clamps each coordinate to the box: Test Failed at test/ode_pso.jl:16
  Expression: (single[1]).position == clamp.(x, lb, ub)
   Evaluated: Float32[5.0, -20.0] == Float32[5.0, -10.0]
   Evaluated: Float32[-10.0, -10.0] == Float32[-10.0, 5.0]
   Evaluated: Float32[10.0, 10.0] == Float32[10.0, -10.0]
Test Summary:                                                          | Fail  Total
parameter_estim_ode! particle update clamps each coordinate to the box |    3      3

Test passing with the fix:

Test Summary:                                                          | Pass  Total
parameter_estim_ode! particle update clamps each coordinate to the box |    3      3

Full Core group (PPS_TEST_GROUP=Core, Pkg.test()) on the merged code:

Regression tests                         |   49     49
Reinitialization tests                   |    0      0
SimpleLBFGS hybrid polish                |   25     25
Algorithm interface tests                |    3      3
ODE parameter estimation particle update |    3      3
BACKEND = CPU                            |   44     44

Runic (--check) and typos report nothing on the three changed files. All CI checks on this PR passed, including CUDA, Core (1, lts, pre, x86), QA, Downgrade, docs and benchmarks.

Not verified: GPU backends locally (CUDA CI passed). I did not re-run the benchmarks/ and examples/ scripts that call parameter_estim_ode!. Their particles now stay inside the box, so their numbers can change.

Round 1 fix (head 15bb81b)

G2: the ODE particle bounds regression runs inside the existing backend loop, including both CPU and CUDA in the CUDA CI job. It selects the backend through test/utils.jl, allocates particle storage with KernelAbstractions.allocate, synchronizes the kernel, and copies results to host memory before asserting the coordinatewise clamp. The three CPU cases remain covered.

Validation used Julia 1.12.7 with JULIA_NUM_THREADS=4 JULIA_NUM_PRECOMPILE_TASKS=4 and timeout 3600 for Julia commands:

  • julia --startup-file=no --project=@runic -m Runic --inplace test/ode_pso.jl test/runtests.jl: exit 0.
  • git diff --check and git diff -- test/ode_pso.jl test/runtests.jl | typos --config .typos.toml -: exit 0, no diagnostics.
  • julia --startup-file=no --project=test -e 'GROUP="CPU"; include("test/ode_pso.jl")': the revised regression fails with the two kernel clamp expressions restored to base behavior, and passes with the fix. Verbatim assertion results and summaries:
Unfixed kernel (exit 1):
   Evaluated: Float32[5.0, -20.0] == Float32[5.0, -10.0]
   Evaluated: Float32[-10.0, -10.0] == Float32[-10.0, 5.0]
   Evaluated: Float32[10.0, 10.0] == Float32[10.0, -10.0]
Test Summary:                                                          | Fail  Total  Time
parameter_estim_ode! particle update clamps each coordinate to the box |    3      3  1.1s

Fixed kernel (exit 0):
Test Summary:                                                          | Pass  Total  Time
parameter_estim_ode! particle update clamps each coordinate to the box |    3      3  0.3s
  • Full Core: PPS_TEST_GROUP=Core GROUP=CPU julia --startup-file=no --project=test test/runtests.jl, exit 0:
Regression tests         | 49 / 49
Reinitialization tests   | 0 total
SimpleLBFGS hybrid polish | 17 / 17
Algorithm interface tests | 3 / 3
BACKEND = CPU            | 47 / 47
  • QA: PPS_TEST_GROUP=QA GROUP=QA julia --startup-file=no --project=test test/runtests.jl stopped before assertions with ArgumentError: Package SafeTestsets not found in current path after activating the QA environment. The declared QA body was then run directly: PPS_TEST_GROUP=QA GROUP=QA julia --startup-file=no --project=test/qa test/qa/qa.jl, exit 0:
Quality Assurance | 21 / 21
Reexport surface  | 27 / 27

Not verified locally: GPU execution (both GPUs are occupied by the shared inference service), end-to-end ODE estimation, benchmarks, and downstream behavior. Docs build omitted because this follow-up only changes tests. CUDA CI at this head is pending; the successful checks described above this note belong to the earlier head. No independent rating of this fix is supplied by its author.

Risk assessment

  • Risk: low

  • Blast radius: one internal kernel (ode_update_particle_states!), used only by ParallelParticleSwarms.parameter_estim_ode!, which is neither exported nor documented. The solve path and the exported algorithms are untouched. There is no public API change.

  • Evidence: the new test fails before the fix and passes after (above); CI is green.

  • Independent review: pending. Both the author and this follow-up ran in Claude Code, so a reviewer from a different harness is still needed.

  • Merge: auto-merge candidate once an independent low-risk review is in.

  • Next review: must come from a non-Codex harness (round fixed by Codex CLI gpt-6.1-sol at head 15bb81b)

Authored in Claude Code 2.1.280 (model: claude-opus-5-5[1m]), session https://claude.ai/code/session_01Vrsu4PESdABpNaxYiTBVfd.

This PR should be ignored until reviewed by @ChrisRackauckas.


🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID 9f322438-56f1-4e04-bf4e-577e8cafc630 (headless, no URL)

Independent review (Codex CLI 0.162.1 gpt-6.1-sol, head 459fe02)

🤖 Automated review section from an AI agent running as Chris Rackauckas's account — not written or reviewed by Chris.

VERDICT: CHANGES
The elementwise clamp fixes the demonstrated coordinate errors. On Julia 1.12.7 with KernelAbstractions 0.9.45, I ran the PR's three assertions against the exact kernel and particle structs extracted into an isolated module: base 8a20cbc failed 3/3; head 459fe02 passed 3/3. The base returned [5, -20], [-10, -10], and [10, 10] instead of [5, -10], [-10, 5], and [10, -10]. The supplied CI snapshot contains 14 successful checks and no failures requiring comparison with main. No linked issues were supplied or listed in the PR metadata.

  1. test/ode_pso.jl:7 — suspected from reading: G2 is unmet because the regression unconditionally selects CPU(). test/runtests.jl:15 runs it outside the backend loop, and no other test calls ode_update_particle_states! or parameter_estim_ode!. Consequently the successful CUDA job does not exercise the changed GPU kernel. Run these cases with CUDA-backed particle storage in the CUDA job, synchronize, and assert the copied-back positions. Retain the CPU cases. Local GPU verification was unavailable: nvidia-smi reported that it could not communicate with the NVIDIA driver. This CI coverage gap needs a test/CI follow-up before merging; no GitHub post was attempted, as instructed.

G1 is satisfied for this clamp change by the independent coordinatewise box projection reference and the failing-before/passing-after assertions. G2 fails as above. G3 is not applicable: the diff does not change shared state, caches, threading, or global defaults. Matched criteria are L2 (regression coverage) and ML2 (numerical particle updates in the specialized ODE estimation path); ML2 sets the level. The ordinary solve kernels are untouched, but ODE estimation trajectories can change, so this is more than a mechanical/internal refactor.

Not verified: Full-package local tests, QA, docs, benchmarks, end-to-end ODE estimation, and GPU execution. The direct package test stopped at an uninstantiated Adapt dependency; successful local evidence uses only the extracted unchanged particle definitions, exact before/after kernel source, and the PR's test body in a scratch environment. Supplied CI statuses were inspected, but live logs and current main CI were not fetched. Tracked repository files were not modified.

🤖 Posted by an AI agent — harness: Codex CLI 0.162.1 · model: gpt-6.1-sol
Conversation: Codex exec session 01a14b2b-cb85-7781-ba88-2706510174ca (no public URL)

Codex CLI gpt-6.1-sol rated it medium-low (ML2) at head 459fe02


🤖 Posted by an AI agent — harness: Codex CLI 0.162.1 · model: gpt-6.1-sol
Conversation: local session 01a14b6e-4c2a-77d2-8d02-6dcd6c6e46d4 (no session URL)

🤖 Automated review section from an AI agent running as Chris Rackauckas's account — not written or reviewed by Chris.

Risk assessment (independent review, Codex Desktop 0.164.0-alpha.11.1 gpt-6.1-sol, slot 1, head 15bb81b)

  • Risk: medium-low (ML1: Corrects particle coordinates on a working ODE estimation path, with independent clamp oracle and discriminating base/head kernel execution.)
  • Verdict: MERGE
  • Session: 01a18db8-7c94-79c2-b327-359a529fd5dc
  • Blocking priorities: none

Review round 1, written by the reviewer named above (Codex Desktop 0.164.0-alpha.11.1 / gpt-6.1-sol) in a session other than the PR author's, and copied here by the posting agent without change to its findings. The posting agent did not re-run the reviewer's commands.

Summary: Clamps each particle coordinate separately instead of lexicographically selecting whole vectors. The actual kernel regression distinguishes base from head and hardware CI passes.

Blast radius: ode_update_particle_states! used by ODE parameter-estimation particle swarms, on CPU and GPU.

Evidence: Executed the actual source-extracted kernel and owned particle type definitions with the committed regression body on KernelAbstractions CPU; head/base results recorded below. The oracle is elementwise clamp, independent of the optimizer. CUDA CI separately reports CPU 53/53 and CUDA 57/57. After pinning KernelAbstractions to its supported 0.9 line, head exit 0 (3/3), base exit 1 (3/3 failed). The initial unconstrained 0.10.4 environment failed in POCL compilation on both revisions and is not used as regression evidence. All 14 checks pass; CUDA job 117490124267 exercises CPU and CUDA suites. G1 uses independently defined clamp expectations; G2 has actual GPU CI execution.

Blocking findings

None.

Non-blocking findings

None.

What I ran

Review resumed from the interrupted Codex Desktop session 01a18dac-69c0-7c62-8ee5-93065b011883. That session froze initial risk choices and independent findings before detailed prior-review comparison; its executed commands and saved outputs were retained. This continuation refreshed every live head/base and check listing. Local runs use Julia 1.13.0, JULIA_NUM_THREADS=4, OPENBLAS_NUM_THREADS=1, an isolated TMPDIR and an isolated writable depot (the existing user depot is a read-only fallback). Environment aliases below identify the isolated checkouts/probes without exposing workstation paths.

git clone --bare https://github.com/SciML/ParallelParticleSwarms.jl.git "$MIRROR"
git fetch origin +refs/pull/140/head:refs/remotes/pr/140 8a20cbcb8e387bd53ac841afb374e7f27e71d776
git worktree add --detach "$CHECKOUT" 15bb81b86034ed9f91be61bd074261341e08fd5c
git rev-parse HEAD
git diff 8a20cbcb8e387bd53ac841afb374e7f27e71d776...HEAD
git log -1 --no-merges --format=%B
gh pr view 140 -R SciML/ParallelParticleSwarms.jl --json number,url,state,isDraft,headRefOid,baseRefName,baseRefOid,title,files,commits,statusCheckRollup
gh pr checks 140 -R SciML/ParallelParticleSwarms.jl
gh run list -R SciML/ParallelParticleSwarms.jl --branch main --event push --limit 5 --json databaseId,headSha,conclusion,url,name
julia --compiled-modules=existing --project="$PROBE_ENV" "$PARTICLE_PROBE" "$HEAD_SOURCE"
julia --compiled-modules=existing --project="$PROBE_ENV" "$PARTICLE_PROBE" "$BASE_SOURCE"
gh run view 39064812363 -R SciML/ParallelParticleSwarms.jl --job 117490124267 --log

Git clone/fetch/checkout/diff/log and live metadata reads exited 0; HEAD matched the full live SHA. gh pr checks exited 0. Check-state snapshot: {"SUCCESS": 14}.

Local output: Executed the actual source-extracted kernel and owned particle type definitions with the committed regression body on KernelAbstractions CPU; head/base results recorded below. The oracle is elementwise clamp, independent of the optimizer. CUDA CI separately reports CPU 53/53 and CUDA 57/57. After pinning KernelAbstractions to its supported 0.9 line, head exit 0 (3/3), base exit 1 (3/3 failed). The initial unconstrained 0.10.4 environment failed in POCL compilation on both revisions and is not used as regression evidence.

CI interpretation: All 14 checks pass; CUDA job 117490124267 exercises CPU and CUDA suites. G1 uses independently defined clamp expectations; G2 has actual GPU CI execution.

Inspected PR job logs:

Recent default-branch runs compared (listed outcomes are not assumed to explain every PR failure):

What I did not verify

Full ODE solve/optimization and local GPU/AMDGPU hardware execution were not run. The local probe isolates the real kernel, rather than loading the entire package. The review is read-only: no upstream code, comments, approval or merge mutation was made. A MERGE verdict is a code-review recommendation; repository CI policy and high-risk human decisions still apply.

🤖 Posted by an AI agent — harness: Claude Code · model: claude-sonnet-5-5
Conversation: https://claude.ai/code/session_01379heiiB35TtywhR7uSU74

ChrisRackauckas and others added 2 commits September 23, 2026 12:07
`ode_update_particle_states!` used `max(position, lb)` / `min(position, ub)` on
SVectors, which compare lexicographically and return one whole vector, so particle
coordinates could leave `[lb, ub]` (and in-box coordinates could be overwritten by
the bound). Use broadcasting, matching `update_particle_states!` in kernels.jl.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.280
Agent-Model: claude-opus-5-5[1m]
Agent-Session: https://claude.ai/code/session_01Vrsu4PESdABpNaxYiTBVfd
Claude-Session: https://claude.ai/code/session_01Vrsu4PESdABpNaxYiTBVfd
Allocate particle storage on the selected backend and copy synchronized results to the host for coordinatewise assertions. Run the regression in the CPU/CUDA backend loop.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Codex <noreply@openai.com>
Agent-Harness: Codex CLI 0.162.1
Agent-Model: gpt-6.1-sol
Agent-Session: local session 01a14b6e-4c2a-77d2-8d02-6dcd6c6e46d4 (no session URL)
@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review October 31, 2026 11:14
@ChrisRackauckas
ChrisRackauckas merged commit 4c6db84 into SciML:main Oct 31, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants