Skip to content

Stabilize sampled eachslice integration with a function barrier - #380

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:int-sampled-slice-barrier
Oct 16, 2026
Merged

ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:int-sampled-slice-barrier

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

_evalrule_general zipped eachslice(data; dims=dim) where dim is a runtime Int, so the slice iterator was type-unstable and the weighted sum loop dynamically dispatched (JET report_opt hit sampled.jl around the accumulate). This PR barriers into _evalrule_slices(slices, weights) so the loop body sees a concrete slice iterator. Results are bitwise identical to master on the same inputs (verified via reinterpret(UInt64, …) on Float64 payloads for Nx4 dim=1 / 4xN dim=2 / 2x2xN dim=3 cases).

Before / after (same machine, JULIA_NUM_THREADS=1 taskset -c 0, @benchmark minima, N=1e5)

case before min before allocs after min after allocs
Nx4 dim=1 trapz 167045 µs 1300013 648 µs 9
2x2xN dim=3 trapz 159613 µs 1400012 1412 µs 9
4xN dim=2 trapz 441 µs 5 481 µs 5

Regression @allocated test (N=10_000, barrier in the test): FAIL on unfixed (5280608 < 2000) → PASS with fix (544 bytes). Also passes on Julia 1.10.12.

Test-group tails

GROUP=Core (julia +1.12, EXIT:0):

Core/sampled_tests.jl | 31 / 31
     Testing Integrals tests passed

GROUP=QA (julia +1.12, EXIT:0):

QA/qa.jl | 27 / 27
     Testing Integrals tests passed

Not verified

  • Full JET report_opt re-run on the patched _evalrule_general path (QA suite covers package JET checks; did not re-run the auditor's standalone JET script).
  • Multi-thread / non-taskset timings; other sampled algs beyond TrapezoidalRule for the dim≠last path under @benchmark.
  • Downstream SciML packages that depend on Integrals.

Reviewer pushback

  • Whether < 2_000 bytes is the right @allocated bound (fixed path is ~544 B; unfixed ~5 MB at N=10k) vs asserting allocation count via BenchmarkTools.
  • Whether _evalrule_slices should gain a blank line / Runic spacing preference relative to neighboring helpers.
  • Whether further specializing on dims::Val is preferred over the barrier for API clarity.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

Independent review: pending

🤖 Generated with Cursor Agent 2026.10.01-14929f9 (model: unknown (Cursor auto)), transcript /home/crackauc/sandbox/goals/performance/jobs/integrals/int-slice/log.txt on amdci2; orchestrated by Claude Code (claude-opus-5-5[1m]) https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Made with Cursor

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #380 (comment)

Risk assessment

  • Risk: low
  • Blast radius: The generic sampled-integration loop used for matrix dim=1, higher-dimensional arrays, and non-fast-scalar-indexing arrays. An internal function barrier changes specialization and allocations; no public API, dependencies, arithmetic, or accumulation order changes.
  • Evidence: All 15 reported PR checks pass. Locally, the head sampled test file passes 31/31 assertions, including its <2,000-byte solve allocation bound. On the PR base, that same allocation assertion fails with 4,616,032 < 2,000. Independent matrix and 3-D kernel probes preserve results exactly and reduce measured allocations from 4,295,696 to 160 bytes and 4,775,696 to 176 bytes respectively.
  • Independent review: codex-cli gpt-6.1-sol rated it low, high confidence: the loop body is preserved verbatim, numerical probes agree exactly, and both the kernel measurements and solve allocation regression pass.
  • Merge: auto-merge candidate — narrow internal correction with targeted regression tests and all reported checks passing; no blocking findings.

Independent risk assessment by codex-cli (gpt-6.1-sol), reviewed head 28144ba72b; dispatched by the fleet master, Claude Code (claude-opus-5-5), https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq

Runtime Int dims made eachslice type-unstable, so the weighted sum
dispatched dynamically and allocated per slice. Barrier into
_evalrule_slices so the loop body is concrete.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Cursor Agent <noreply@cursor.com>
Agent-Harness: Cursor Agent 2026.10.01-14929f9
Agent-Model: unknown (Cursor auto)
Agent-Session: local session, transcript /home/crackauc/sandbox/goals/performance/jobs/integrals/int-slice/log.txt on amdci2
@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

PR: #380 (head 28144ba, base origin/master 63b17e8). It changes src/sampled.jl by +4/-1 and adds one testset. The change moves the existing loop body of _evalrule_general into a new internal helper, _evalrule_slices(slices, weights), which acts as a function barrier. The arithmetic and the order of operations are unchanged. No exports or new dependencies are added and no public API changes.

Blocking findings

None.

Non-blocking findings

  1. The new test checks allocations only, not values (test/sampled_tests.jl:85-94). This is confirmed by reading and running. The general (dim ≠ last) path still has no analytic value check anywhere in the suite. The only existing dim=1 check is the "Caching interface" testset, which compares solve! against solve on the same path. My probes (see "What I ran") show the results are correct and bitwise identical to master, so this is a coverage gap and not a defect. A cheap way to harden the test: in the same testset, add @test solve(prob, alg).u ≈ solve(SampledIntegralProblem(permutedims(Y), x; dim = 2), alg).u. That compares the general path against the separate fast path.
  2. Only the Nx4 dim=1 matrix case is covered by the regression test. The N-d fallback evalrule(::AbstractArray, …), e.g. 2x2xN with dim=3, gets the same fix. I measured it at 6,720,624 B on master versus 560 B on the PR, but no test guards it. Looping the testset over (rand(N,4),1) and (rand(2,2,N),3) would cover both.
  3. The PR body claims the QA suite covers the JET checks for this path. It does not (checked by reading). test/qa/qa.jl:109-121 runs @report_opt only on 1-D SampledIntegralProblem vectors, which take the fast path in evalrule(::AbstractVector, …). So I ran JET myself (below).
  4. There is no linked issue. closingIssuesReferences is empty and the body links no tracking issue, so I could not check for sibling work in flight.
  5. The allocation bound is not derived. The < 2_000 bound is about 3.7× the observed 544 B. Master allocates about 5.3 MB, so the bound cannot pass with the bug present. I confirmed it holds on 1.12, on 1.10.12, and with -t4 --check-bounds=yes --code-coverage=user, so it is not fragile in practice.
  6. Pre-existing issue, not caused by this PR (seen while probing): SimpsonsRule on a non-uniform grid is not exact for a cubic. For ∫₀² (t³−t) dt = 2 with 1001 random points, I got relative error ≈4.7e-9, identically on master and the PR. I did not investigate whether this is expected for the nonuniform weights Integrals uses. It is out of scope here.

What I ran

All runs used TMPDIR and scratch envs under ../scratch. env_pr dev's the PR checkout and env_master dev's a local clone at origin/master 63b17e8.

  • Value probes (scratch/probe.jl), confirmed by running on both master and the PR. TrapezoidalRule on a linear integrand and SimpsonsRule on a cubic integrand were checked against analytic values (rtol 1e-12; 1e-7 for non-uniform Simpson, because of item 6). Grids were uniform and non-uniform; layouts were Nx4 dim=1, N×2×2 dim=1, 2×2×N dim=3 and 2×N×2 dim=2. Each layout was also compared to the transposed dim=2 fast path at rtol 1e-13. Also covered: the Unitful axis on dim=1 (== [2.0,2.0]u"m"), BigFloat, and the empty-input throw. Everything passed on both.

  • Bitwise comparison (scratch/cmp.jl). I serialized 20 payloads from both runs: the 4 layouts × 2 algs × uniform/non-uniform grids, plus Float32, ComplexF64, Unitful and BigFloat. Output: 20 payloads compared; mismatches: String[]. Typed isequal matches, so the PR's bitwise-identical claim reproduces.

  • @allocated solve(...), N=10,000, after warmup, master → PR:

    case master PR
    Nx4 dim=1 5,280,608 B 544 B
    2x2xN dim=3 6,720,624 B 560 B
    ComplexF64 Nx4 dim=1 5,280,640 B 576 B
    Unitful Nx4 dim=1 5,280,608 B 544 B
    4xN dim=2 448 B 448 B

    Trapezoidal and Simpson gave identical numbers.

  • Fail before / pass after. Running the PR's test/sampled_tests.jl against master code fails with Evaluated: 5280608 < 2000. Against the PR code, all testsets pass. That holds on Julia 1.12, with -t4 --check-bounds=yes --code-coverage=user, and on Julia 1.10.12.

  • JET (scratch/jet.jl, output in scratch/jet_pr.txt):

    • @report_opt _evalrule_slices(eachslice(data; dims=d), w) reports "No errors detected" for all 3 layouts.
    • @report_opt target_modules=(Integrals,) solve(p, TrapezoidalRule()) leaves 2 reports. One is the intended single dynamic dispatch at the barrier, sampled.jl:104. The other is the build_solution kwcall at sampled.jl:170, which is outside this change.
  • Benchmark (scratch/bench.jl, BenchmarkTools minima, -t1, N=1e5, TrapezoidalRule), master → PR:

    case master PR
    Nx4 dim=1 91.5 ms, 1,300,013 allocs 0.60 ms, 9 allocs
    2x2xN dim=3 95.8 ms, 1,400,012 allocs 0.77 ms, 9 allocs
    4xN dim=2 0.58 ms, 5 allocs 0.58 ms, 5 allocs

    The allocation counts match the PR table exactly. Absolute times differ because this is a different machine, but the ratios are consistent.

  • Lint and CI. julia +1.12 --project=@runic -m Runic --check --diff on both files exited 0, and typos on the diff exited 0. gh pr checks 380 shows all checks passing (Core lts/1/pre/x86, AD, ADBatch, QA, docs, Runic, typos, benchmark).

  • Rule audit (by reading):

    • The repo has no AGENTS.md or CLAUDE.md, so I applied the global rules.
    • The diff adds no comments.
    • It is a single concern with no new dependencies and no non-public dependency API.
    • The new name is _-prefixed and internal, so it needs no docstring.
    • The commit carries the attribution trailers.
  • Cleanup. My coverage run wrote *.94870.cov files into the checkout. I removed exactly those files, and git status --short is now clean. No Julia processes of mine are left running.

What I did not verify

  • I did not run the full GROUP=Core or GROUP=QA locally. I relied on sampled_tests.jl run directly plus the green CI runs. The AD groups do not touch evalrule: the Mooncake extension only marks init_cacheval zero-derivative, and nothing else differentiates through the sampled path.
  • I did not test GPU or other non-fast_scalar_indexing arrays, which also route through _evalrule_general. By reading, the barrier doesn't change their semantics.
  • I did not check downstream packages.
  • I did not check sibling PRs, because no tracking issue is linked.

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review October 16, 2026 04:24
@ChrisRackauckas
ChrisRackauckas merged commit e048f68 into SciML:master Oct 16, 2026
15 checks passed
ChrisRackauckas referenced this pull request Oct 17, 2026
Bump version to 5.5.4
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.301
Agent-Model: claude-opus-5-5
Claude-Session: https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq
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