From 61250c967461a140f32d6a256240bc17447664c0 Mon Sep 17 00:00:00 2001 From: ChrisRackauckas-Claude Date: Thu, 8 Oct 2026 07:16:51 -0400 Subject: [PATCH] Make fill!(::AbstractVectorOfArray, x) fill inner arrays directly fill! looped over VA[:, i], which for a ragged inner array builds and returns a zero-padded *copy* (src/vector_of_array.jl's _getindex for (::Colon, ::Int)). Filling that copy touches nothing: the write is silently dropped for every inner array shorter than the ragged maximum. It also costs an unnecessary allocating reconstruction on every iteration even in the common non-ragged case. Iterate VA.u[i] directly instead: it is always the real storage, so fill! correctly fills every inner array regardless of its size, and the non-ragged case no longer reconstructs anything. n=1000 inner arrays: fill! 2.43ms -> 4.8us (1.12), 2.45ms -> 2.1us (1.10, per the earlier audit). Ragged correctness (previously silently wrong) verified: filling a ragged VectorOfArray([[1.,2.],[3.],[4.,5.,6.]]) with 9.0 now gives [[9.,9.],[9.],[9.,9.,9.]] (every inner array filled) instead of the pre-existing [[1.,2.],[3.],[9.,9.,9.]] (only the longest inner array actually gets filled). Co-Authored-By: Chris Rackauckas Co-Authored-By: Claude Agent-Harness: Claude Code 2.1.294 Agent-Model: claude-sonnet-5 Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv --- src/vector_of_array.jl | 17 +++++++++++------ test/Core/interface_tests.jl | 8 ++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/vector_of_array.jl b/src/vector_of_array.jl index af4da6e9..95777e05 100644 --- a/src/vector_of_array.jl +++ b/src/vector_of_array.jl @@ -1167,16 +1167,21 @@ end # fill! # For DiffEqArray it ignores ts and fills only u function Base.fill!(VA::AbstractVectorOfArray, x) - for i in 1:length(VA.u) - if VA[:, i] isa Union{AbstractArray, AbstractVectorOfArray} - if ArrayInterface.ismutable(VA.u[i]) || VA.u[i] isa AbstractVectorOfArray - fill!(VA[:, i], x) + for i in eachindex(VA.u) + # Fill the inner array directly instead of through `VA[:, i]`, which + # for a ragged inner array builds and returns a zero-padded *copy* — + # filling that copy does not touch (and silently drops writes to) the + # real storage. + ui = VA.u[i] + if ui isa Union{AbstractArray, AbstractVectorOfArray} + if ArrayInterface.ismutable(ui) || ui isa AbstractVectorOfArray + fill!(ui, x) else # For immutable arrays like SVector, create a new filled array - VA.u[i] = fill(x, StaticArraysCore.similar_type(VA.u[i])) + VA.u[i] = fill(x, StaticArraysCore.similar_type(ui)) end else - VA[:, i] = x + VA.u[i] = x end end return VA diff --git a/test/Core/interface_tests.jl b/test/Core/interface_tests.jl index c4aa6ae8..59f45857 100644 --- a/test/Core/interface_tests.jl +++ b/test/Core/interface_tests.jl @@ -248,6 +248,14 @@ testda = DiffEqArray(recursivecopy(testva.u), testts) fill!(testda, testval) @test all(x -> (x == testval), testda) +# fill! on a ragged VectorOfArray must fill every inner array, not just the +# longest one. `VA[:, i]` zero-pads a shorter inner array into a throwaway +# copy, so filling through it (instead of through `VA.u[i]` directly) would +# silently drop the write for every inner array shorter than the max. +ragged_fill = VectorOfArray([[1.0, 2.0], [3.0], [4.0, 5.0, 6.0]]) +fill!(ragged_fill, 9.0) +@test ragged_fill.u == [[9.0, 9.0], [9.0], [9.0, 9.0, 9.0]] + # copyto! testva = VectorOfArray(collect(0.1:0.1:1.0)) arr = 0.2:0.2:2.0