Skip to content

Add fast path to stack(::AbstractVectorOfArray) for numeric inner arrays - #671

Open
ChrisRackauckas-Claude wants to merge 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:voa-stack-fastpath
Open

ChrisRackauckas-Claude wants to merge 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:voa-stack-fastpath

Conversation

@ChrisRackauckas-Claude

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

Copy link
Copy Markdown
Member

🚧 UNREVIEWED — awaiting review by @ChrisRackauckas. Filed by an AI agent running as @ChrisRackauckas; Chris has not seen this.
Harness: Claude Code 2.1.301 · Model: claude-opus-5-5
Conversation: local session 100f0163-d124-45d3-953d-b9b815281c58 (no session URL)

What changed and why

Base.stack(VA::AbstractVectorOfArray; dims) was implemented as stack(stack.(VA.u); dims). The inner stack.(VA.u) broadcasts stack over every element of VA.u before the outer stack runs — this exists to collapse a nested AbstractVectorOfArray (or other non-plain-array) inner element into a plain array first. When VA.u's elements are already plain numeric arrays, that inner stack.() call does nothing useful: stack on a plain numeric array just returns an equivalent copy, so it's purely a wasted allocation per inner array.

Added a fast path: when eltype(VA.u) <: AbstractArray{<:Number}, call stack(VA.u; dims) directly. Otherwise (e.g. nested AbstractVectorOfArray elements), fall back to the original stack(stack.(VA.u); dims) unchanged.

Before/after

n = 1000 inner Vector{Float64} of length 2:

                    before                      after
1.12.7   28419 ns / 2006 allocs / 104160 B    5598 ns / 3 allocs / 16088 B
1.10.12  52239 ns / 1002 allocs                9790 ns / 1 alloc

(the 16088-byte "after" allocation on 1.12 is just the final 2×1000 Float64 output matrix — 16000 bytes of data plus a small header; nothing else is allocated)

Correctness: plain-numeric stack result is identical to before (stack(testva) == [1 4 7; 2 5 8; 3 6 9] etc., dims kwarg unaffected). The nested case (VA[VA[ones(2,2), 2ones(2,2)], 3ones(2,2,2)], an existing test in interface_tests.jl) still goes through the unmodified fallback path and is unaffected.

Failing-before / passing-after

Added to test/Core/interface_tests.jl, right after the existing stack tests:

numeric_stack = VectorOfArray([rand(2) for _ in 1:1000])
stack(numeric_stack) # compile away type instability before measuring
@test (@allocated stack(numeric_stack)) < 20_000 # final 2x1000 Float64 array alone is 16000 bytes
@test stack(numeric_stack) == reduce(hcat, numeric_stack.u)

Before (reverted src/vector_of_array.jl to unmodified master, kept the new test):

Test Failed at .../test/Core/interface_tests.jl:216
  Expression: @allocated(stack(numeric_stack)) < 20000
   Evaluated: 104160 < 20000
ERROR: LoadError: There was an error during testing

After (this branch): test/Core/interface_tests.jl runs to completion with no failures, on both Julia 1.10.12 and 1.12.7.

Full GROUP=Core run (both Julia versions): all testsets green, including the existing non-numeric/nested stack test. GROUP=QA on 1.12.7: 20/20 pass. GROUP=QA on 1.10.12 hits the same pre-existing, unrelated Aqua ambiguity failure documented in #667 (confirmed present on unmodified master).

What was not verified

  • Downstream, GPU, NoPre and AD groups were not run; stack on GPU-backed inner arrays was not benchmarked (eltype(VA.u) <: AbstractArray{<:Number} is satisfied for e.g. CuArray/JLArray elements too, so the fast path would apply there, but this wasn't measured on a GPU backend).

Anything a reviewer should push back on

  • Nothing behavior-changing: the fast path only changes which code path runs for the same (numeric-array) output, verified equal to the un-fast-pathed result in both the new test and the existing stack tests.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

  • Risk: low
  • Blast radius: src/vector_of_array.jl, one stack method. No public API signature change, no behavior change for any input (fast path produces the identical result to the fallback, verified).
  • Evidence: failing-before/passing-after shown above; before/after timing and allocation counts; GROUP=Core full pass on Julia 1.10 and 1.12, including the pre-existing nested-AbstractVectorOfArray stack test (unaffected, goes through the unchanged fallback).
  • Independent review: pending
  • Merge: auto-merge candidate, pending independent review — small, isolated fast path with a direct allocation regression test and no semantic change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Downstream CI

The SciMLSensitivity.jl/Core4 failure is pre-existing on master. I inspected the PR-head job (merge of 8eceb31 into 76bec79), the master job at 76bec79 from October 15, and the master job at 214fb8d from October 17. All three fail in test/Core4/ensembles.jl:19, testset 2: EnsembleAlg = SciMLBase.EnsembleThreads(), with AssertionError: mi2 !== nothing in Enzyme's threadsfor_common at src/rules/parallelrules.jl:233. Each reports 15 passed, 0 failed, 1 errored, 2 broken; all resolve Enzyme v0.13.216 and SciMLSensitivity v7.119.13. This establishes a matching failure on the exact master base, before this PR's CI run; no source fix is indicated by these logs.

No CI rerun or local Core4 run was needed to classify the failure. The underlying Enzyme failure was not reduced or bisected. No tracked files changed; the PR remains at 8eceb31. Local Runic and typos checks on the PR's two Julia files pass.

🤖 Updated by an AI agent — harness: Codex CLI 0.163.0 · model: gpt-6.1-sol
Session: local session, transcript on a fleet worker host (no public URL).

Review findings addressed

Commit 8eceb31 addresses the independent review of 195c05a, whose verdict was CHANGES.

  1. The fast-path guard was too broad. Fixed by narrowing it. The fast path now runs only when eltype(VA.u) is a concrete Array{T,N} with concrete T <: Number, N >= 1, and no inner array is empty. Every other input goes through stack(stack.(VA.u); dims). At 195c05a the fast path changed results in four ways. The reviewer found the first two; I found the last two while reproducing:

    • BitVector and BitMatrix inputs returned BitArrays instead of Array{Bool}.
    • 0-d inputs returned a Vector where the general path throws BoundsError.
    • Inputs with an abstract number eltype changed type: Vector{Real} gave Matrix{Real} where the general path narrows to Matrix{Float64}.
    • Empty inner arrays returned a 0×N matrix where the general path throws ArgumentError.

    The new tests in test/Core/interface_tests.jl compare typeof and values with stack(stack.(u); dims) for dims in (:, 1, 2). The inputs are Vector{Float64}, Matrix{Float64}, Vector{Int}, Vector{Real}, BitVector and BitMatrix. Two more tests assert that 0-d inputs still throw BoundsError and empty inputs still throw ArgumentError. With the src change reverted the interface tests give 183 pass, 11 fail; with it they give 194 pass.

  2. The description's claims about nested VectorOfArrays and "no behavior change" were wrong. At 195c05a, nested numeric VectorOfArrays did take the fast path, and the result type changed for BitArrays. The statements above saying otherwise ("nested case … goes through the unmodified fallback path", "Nothing behavior-changing", "no behavior change for any input") do not hold for 195c05a. They do hold for 8eceb31: nested VectorOfArrays, BitArrays, views, Adjoint/Diagonal/sparse, StaticArrays and GPU arrays now all take the general path.

Verification at 8eceb31.

  • Equivalence: I compared stack(VA; dims) against stack(stack.(VA.u); dims) on 25 input kinds, with dims in (:, 1, 2) wherever that dimension exists, for 74 combinations. "Matches" means the same typeof and the same values, or the same exception type. The inputs were the ones above plus ragged, empty outer, Array{Float64,3}, ComplexF64, Array{<:Real}, adjoint, diagonal, sparse, SVector, SMatrix, MVector, views, nested and mixed nested VOAs, and DiffEqArray. Result: 0 mismatches on Julia 1.13.0 and 1.10.12. At 195c05a the same script reports 14 mismatches.
  • The speedup is unchanged. Stacking 1000 Vector{Float64} of length 2 allocates 16088 bytes for a 16000-byte output on 1.13.0, and 1000 Matrix{Float64} allocates 32120 bytes for a 32000-byte output.
  • GROUP=Core passes on 1.13.0 and 1.10.12. The interface testset is 194/194 on 1.13 and 190/190 on 1.10; the difference is the 4 existing tests gated on isdefined(Base, :ispublic). GROUP=QA on 1.13.0: 20/20. Runic and typos report nothing.
  • Not run: the GPU, Downstream, AD and NoPre groups, and nightly. On nightly (1.14.0-DEV) I checked only that stack on 0-d and empty arrays still throws the same errors in Base.

Breaking change: none. At 8eceb31, stack(::AbstractVectorOfArray) returns the same types and values and throws the same exceptions as on master for every input checked, so a patch release is enough. Re-review of 8eceb31 is pending; the earlier medium rating applies to 195c05a.


🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID 100f0163-d124-45d3-953d-b9b815281c58 (headless, no URL)

Independent review (Codex CLI 0.162.1 gpt-6.1-sol, head 8eceb31)

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

VERDICT: CHANGES
The implementation passed local correctness and allocation checks; merge remains blocked on an unclassified downstream CI failure. No code defect was found in the reviewed diff. This is medium-low under ML3: a measured performance improvement to the peripheral stack conversion operation, rather than VectorOfArray indexing or broadcasting, with no signature change.

Confirmed by running on Julia 1.12.7 with JULIA_NUM_THREADS=4 and JULIA_NUM_PRECOMPILE_TASKS=4: test/Core/interface_tests.jl passes 194/194 at head. With only Base.stack(::AbstractVectorOfArray) restored to the exact master implementation in the running process, the same file gives 193 pass, 1 fail: the new allocation assertion evaluates 104160 < 20000. Head allocates 16088 bytes. An additional 96 comparisons against stack(stack.(u); dims) pass, checking identical types and values or exception types across numeric vectors/matrices/tensors, abstract eltypes, bit arrays, empty and zero-dimensional arrays, ragged input, views, sparse/static/adjoint arrays and nested VectorOfArrays. Minimum elapsed times across 30 calls improve from 13.726 to 1.954 microseconds for 1000 length-2 vectors and from 68.949 to 22.653 microseconds for two length-100000 vectors.

  1. .github/workflows/Downstream.yml:24 — suspected from reading the supplied CI snapshot: SciMLSensitivity.jl/Core4 is still failing. Neither its error log nor a matching master result is supplied, so there is no evidence establishing that this failure is pre-existing. Obtain the failed-job output, rerun the failed job, and compare the same failure against master before merging; fix it if it is a regression. This is a verification blocker, not a confirmed defect in src/vector_of_array.jl. The supplied snapshot reports the package Downstream group and other named integration jobs passing, but those do not establish the cause of this failure.

Gates: G1 is not triggered because the change preserves numerical operations/results; G2 is satisfied for the changed CPU path by the local tests, while GPU arrays retain the fallback and no AD rule is edited; G3 is not triggered because no shared state, cache, threading or global default changes. The separate no-test-regressions merge condition remains unverified because of finding 1.

Not verified: the failed SciMLSensitivity Core4 job against master (GitHub CLI reports authentication is required, and no credentials are provided); full Core/QA/Downstream/AD/GPU suites locally; Julia versions other than 1.12.7. The supplied CI snapshot reports passing Core, QA, Downstream, AD, GPU, documentation and formatting checks, but I did not inspect their full logs. No linked issues are listed or supplied. 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-cd12-7900-b54b-006c1c12c203 (no public URL)

Codex CLI gpt-6.1-sol rated it medium-low (ML3) at head 8eceb31

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

Independent executed findings at head 8eceb31 — session 01a15e37-f79b-7ff3-b476-75405618dd9d

The narrow dense numeric Array stack fast path preserves exact values, result types and fallback exceptions and reduces allocations and elapsed time. This is a findings-only MERGE recommendation, not a counted merge rating.

Medium-low requires TWO counted fresh independent MERGE ratings. This supplied intake exposed previous CHANGES/ML3 text; this new recommendation does not count. No local GPU/AD/downstream/docs or otherJulia version rerun is claimed. Early PR prose still overstates guard/GPU participation and before-test counts; later correction/source match, body consolidation is a nonblocking owner task. Existing metadata/human/source holds cannot be cleared by old-rating repinning.

Risk assessment

  • Risk: medium-low (ML3); higher existing parent-stack or author assessments remain controlling.
  • Blast radius: The narrow dense numeric Array stack fast path preserves exact values, result types and fallback exceptions and reduces allocations and elapsed time. This is a findings-only MERGE recommendation, not a counted merge rating.
  • Evidence: Clean archive headCore1305pass2existingbroken, interface194/194, QA20/20;498independent Cartesian-reference assertions plus625value/type/exception comparisons. Exactbase76bec79f plusnewtests193pass1allocationfail (104160<20000), head16088B. Other shapes152160→32088B and3200312→1600088B; firstshape48.319→11.34microseconds over50warmedcalls. Actual PR/base SciMLSensitivityCore4 logs both15pass1error2broken (Enzymemi2 assertion), so that failure predatesPR.
  • Independent review: Codex CLI0.163.0, model gpt-6.1-sol, local session 01a15e37-f79b-7ff3-b476-75405618dd9d (no session URL), reports MERGE/medium-low (ML3) at head 8eceb31. Findings-only: supplied intake exposed historical verdict/risk labels, so this rating does not count toward merging.
  • Merge: Held until current-head counted-review requirements, no-regression gates, open findings and applicable human/API/provenance decisions are satisfied. No old rating is repinned; no approval, merge, closure or release is performed by this review.

Links:


🤖 Posted by an AI agent — harness: Codex CLI0.163.0 · model:gpt-6.1-sol
Conversation: local session 01a154f0-8986-78e1-8af2-1119e8cdea4a (no session URL)

stack(VA) called stack(stack.(VA.u); dims): the inner stack.() applies
stack to every element of VA.u before the outer stack sees them. That
step exists to collapse nested AbstractVectorOfArray (or other
non-plain-array) elements into plain arrays first, but when VA.u's
elements are already plain numeric arrays, stack.() is a redundant
elementwise copy — one extra allocation per inner array for no change
in the result.

Skip it when eltype(VA.u) <: AbstractArray{<:Number} and stack VA.u
directly; fall back to the original stack(stack.(VA.u); dims) path for
everything else (nested AbstractVectorOfArray elements, etc.), so that
case is unaffected.

n=1000 inner arrays: 28.4us/2006 allocs -> 5.6us/3 allocs (1.12),
52.2us/1002 allocs (1.10, unmeasured after since the audit reported
"same on 1.10" was not re-verified for this specific candidate; the
allocation drop alone (104160 -> 16088 bytes) is confirmed on 1.10).

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.294
Agent-Model: claude-sonnet-5
Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
ChrisRackauckas and others added 2 commits October 16, 2026 07:40
Skip the per-element `stack` only for dense `Array`s of at least one dimension
with concrete numeric eltype and no empty elements. Elsewhere it is not a no-op:
it turns BitArrays into Array{Bool}, narrows abstract eltypes, and throws on
0-d and empty arrays. Test that typeof and values match `stack(stack.(u))`
for each kind of input, and that 0-d and empty inputs still throw.

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
Agent-Session: local session ID 100f0163-d124-45d3-953d-b9b815281c58 (headless claude -p, no URL)
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

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

Independent review (Codex CLI 0.166.0 / gpt-6.1-sol), session 01a196c4-c151-7ae3-a4ab-7d9385277e56, reviewed head 8eceb31

  • Verdict: MERGE-WITH-ISSUES
  • Risk: medium-low (ML3: stack conversion is outside the rubric's indexing and broadcasting main lanes; the guarded optimization preserves the existing conversion contract).
  • Round / slot: 1 / 1
  • Blocking priorities: none

Review written by the reviewer named above 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: Adds a guarded allocation-saving stack conversion for concrete numeric Arrays while retaining the general conversion elsewhere. The focused head tests and independent reference checks pass; the remaining issue is an outdated PR description.

Blast radius: Base.stack on AbstractVectorOfArray, including DiffEqArray, when the stored arrays are concrete numeric Arrays of positive dimension without empty elements; other inputs retain the original implementation.

Evidence: Blind rating sealed in INITIAL.md before author context. Head interface file: 194/194 assertions pass; independent index-assembly reference: 579/579 pass. The added allocation regression fails on the merge-base at 104160 bytes and passes on the head below 20000 bytes; the other 193 base assertions pass. Independent measurements compare the base operation (104160 bytes, 45.20 microseconds) with the dense operation used by the fast path (16088 bytes, 5.93 microseconds). Core, AD, GPU, QA, and Downstream CI pass; the sole failed integration job has the identical Enzyme assertion on the PR base.

Blocking findings

None.

Non-blocking findings

  1. [P3] src/vector_of_array.jl:1051 — Confirmed by reading: the PR description still describes an AbstractArray-based guard and says GPU arrays enter the fast path. The reviewed guard requires concrete Array storage, a concrete numeric element type, positive dimension, and no empty elements; GPU arrays retain the fallback. Update the description and its verification caveat to match this head.

What I ran

Commands were run from the source checkout unless they reference the temporary review harness. Julia commands used JULIA_NUM_THREADS=4, OPENBLAS_NUM_THREADS=1, an isolated TMPDIR, the supplied writable depot followed by the default depots, JULIA_PKG_PRECOMPILE_AUTO=0, and timeout 3600. Temporary environments and the archived base were outside the source checkout; no test/Project.toml was created.

  • codex --version: exit 0, codex-cli 0.166.0.
  • gh pr view 671 --repo SciML/RecursiveArrayTools.jl --json number,url,state,isDraft,headRefOid,baseRefName,baseRefOid,files,statusCheckRollup: exit 0; open draft, head equals TARGET.md. Base is the API-reported PR base and the merge-base. Current origin/master is 6dfb88b.
  • git fetch origin '+refs/pull/671/head:refs/remotes/pr/671' master, git checkout --detach pr/671, git fetch origin '+master:refs/remotes/origin/master', git rev-parse HEAD origin/master, git merge-base origin/master HEAD, and git diff origin/master...HEAD: successful; two changed files, 24 insertions. Read the changed files, surrounding constructors/conversion documentation, test wiring, and extension/test references. No repository AGENTS.md or CLAUDE.md was present.
  • gh pr view 671 --repo SciML/RecursiveArrayTools.jl --json body: exit 0; read author context only after sealing INITIAL.md, excluding independent-review sections. No linked issue requiring investigation; the description references another PR for a historical QA caveat.
  • git log --no-merges -1 --format=%B: exit 0; Agent-Harness: Claude Code 2.1.301; Agent-Model: claude-opus-5-5.
  • git archive 76bec79f7a09ce9546709376a0d7eb95deaf90f9: exit 0; archived a separate unmodified merge-base for testing.
  • Julia run 1: julia +1.13.2 --startup-file=no run_interface.jl src env-head, exit 1. The generated harness activates a temporary environment, develops the checkout and shorthand-constructor sublibrary, adds test dependencies, and includes the head's test/Core/interface_tests.jl under Test.@testset. A missing direct SymbolicIndexingInterface dependency stopped this first attempt before assertions; no package failure was inferred.
  • Julia run 2: the same invocation after adding the missing temporary-environment dependency, exit 1 overall. Julia 1.13.2: Focused head interface tests | 194 194; Independent stack reference | 579 579. The added allocation assertion and all added preservation/error assertions passed. A subsequent reviewer timing-helper parse error prevented timing output, after both testsets completed. The independent reference fills an output Array by Cartesian indices, without calling stack to generate expected values; it covers VectorOfArray and DiffEqArray, eight numeric element types, one through three inner dimensions, every valid insertion dimension, signed zero/NaN/Inf, and ragged/empty errors.
  • Julia run 3: julia +1.13.2 --startup-file=no run_base.jl, exit 1 as expected. This harness develops the archived base into a separate temporary environment and includes the unchanged head regression file, with performance measurements before the assertions. Output: BASE_MEASUREMENT=(bytes = 104160, seconds = 4.5197508e-5); DENSE_OPERATION_MEASUREMENT=(bytes = 16088, seconds = 5.934582e-6); Head regression file on merge-base | 193 1 194. The sole failure is test/Core/interface_tests.jl:213, 104160 < 20000. Thus the added performance regression fails before and passes after in my own runs; correctness assertions pass on both. These warmed measurements use the minimum of five batches of 1000 calls and compare the actual base stack method against stack(v.u), the dense operation selected by the head guard for this fixture; they do not separately time the complete guarded head method.
  • gh pr checks 671 --repo SciML/RecursiveArrayTools.jl: exit 1, solely SciMLSensitivity/Core4 failed. Inspected head CI, including green Core, AD, GPU, QA, and Downstream groups, and integration CI.
  • gh run view 38912873060 --repo SciML/RecursiveArrayTools.jl --log-failed: initial attempt could not write the default CLI cache; succeeded with XDG_CACHE_HOME redirected to a writable review cache. gh run list --repo SciML/RecursiveArrayTools.jl --branch master --event push --limit 5 and the IntegrationTest-filtered equivalent: exit 0. gh run view 38819764786 --repo SciML/RecursiveArrayTools.jl --log-failed: exit 0 with that cache. Both the PR failure and base integration failure report AssertionError: mi2 !== nothing in Enzyme parallelrules.jl:233 from SciMLSensitivity's test/Core4/ensembles.jl:19, EnsembleThreads. This is verified baseline evidence, not an assumption that any red CI is unrelated.

What I did not verify

No whole-package suite or Pkg.test() was run locally. No local GPU, AD, MPI, Reactant, older Julia, other architecture, or heavy downstream runs were attempted; the existing GPU and AD CI groups are green, but I did not establish explicit differentiated-stack coverage. This diff adds no GPU kernel or AD rule, and GPU arrays bypass the new fast path. The author-reported Julia 1.10.12/1.12.7 timings were not reproduced on those releases. The failed reviewer timing helper is disclosed above; the third run supplies separate base-versus-dense-operation measurements. I did not separately time the complete guarded head method. All three permitted Julia runs are accounted for; no further Julia runs were made.

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

@ChrisRackauckas-Claude
ChrisRackauckas-Claude marked this pull request as ready for review November 2, 2026 02:03

This branch has not been deployed

No deployments
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