Fix multi-dimensional VectorOfArray broadcast - #374
Conversation
the previous implementation for VoA with Vector parent arrays was `VectorOfArray([similar(VA[:, i], T) for i in eachindex(VA.u)])`. This creates a VoA by calling `similar` on each entry of `VA`, but necessitated a different implementation for VoA with multi-dimensional parent arrays. Creating a new VoA by broadcasting `similar` over the parent array is (I believe) equivalent to previous implementations for VoA with both Vector and Array parents.
the previous implementation for VoA with Vector parent arrays was `VectorOfArray([similar(VA[:, i], T) for i in eachindex(VA.u)])`. This creates a VoA by calling `similar` on each entry of `VA`, but necessitated a different implementation for VoA with multi-dimensional parent arrays. Creating a new VoA by broadcasting `similar` over the parent array is (I believe) equivalent to previous implementations for VoA with both Vector and Array parents.
…/RecursiveArrayTools.jl into jc/fix_bcast_multidim_VoA
This reverts commit f4675f2.
| if parent isa AbstractVector | ||
| # this is the default behavior in v3.15.0 | ||
| N = narrays(bc) | ||
| return VectorOfArray(map(1:N) do i | ||
| copy(unpack_voa(bc, i)) | ||
| end) | ||
| else # if parent isa AbstractArray | ||
| return VectorOfArray(map(enumerate(Iterators.product(axes(parent)...))) do (i, _) | ||
| copy(unpack_voa(bc, i)) | ||
| end) | ||
| end |
There was a problem hiding this comment.
Side note, this entire block can be replaced by the second branch
return VectorOfArray(map(enumerate(Iterators.product(axes(parent)...))) do (i, _)
copy(unpack_voa(bc, i))
end)since enumerate(Iterators.product(axes(parent)...))) do (i, _) basically recovers map(1:N) do i for an AbstractVector.
I left the old behavior in since I wasn't sure if it would break something upstream or change performance (and it's easier to read).
| function Base.similar(vec::VectorOfArray{ | ||
| T, N, AT}) where {T, N, AT <: AbstractArray{<:AbstractArray{T}}} | ||
| return VectorOfArray(similar(Base.parent(vec))) | ||
| return VectorOfArray(similar.(Base.parent(vec))) |
There was a problem hiding this comment.
Broadcasting similar here avoids some #undef issues.
There was a problem hiding this comment.
Side note: this implementation could replace these 3 functions
RecursiveArrayTools.jl/src/vector_of_array.jl
Lines 715 to 729 in eacbe3f
However, doing so breaks these tests because similar can't be called on <:Real values:
RecursiveArrayTools.jl/test/basic_indexing.jl
Lines 221 to 229 in eacbe3f
These tests don't seem correct to me; the parent array
x doesn't have underlying data structure Vector{AbstractArray{T}}, so it shouldn't be a valid edge case. Maybe I'm missing something?
There was a problem hiding this comment.
Numbers are also allowed. That's used a lot downstream.
There was a problem hiding this comment.
We can special-case that case where the elements are numbers, but we cannot throw it out.
|
Applying formatting introduces a bunch of spurious file changes so I'm skipping it for now. |
|
Thanks for the quick review! |
2 participants
Addresses #373
The fix just involves modifying
Base.copyforVectorOfArraywith multi-dimensional parent arrays. This could replace the oldBase.copyimplementation, but I left the old implementation there just in case it changes behavior upstream.