-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix multi-dimensional VectorOfArray
broadcast
#374
Changes from all commits
0b93c51
67da057
864a463
b9a6f23
af17d47
fd61439
2973fc6
6549aaa
8ece560
f4675f2
a0770e4
b7f9c84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,16 +28,15 @@ | |
returns a vector of the series for each component, that is, `A[i,:]` for each `i`. | ||
A plot recipe is provided, which plots the `A[i,:]` series. | ||
|
||
There is also support for `VectorOfArray` with constructed from multi-dimensional arrays | ||
|
||
There is also support for `VectorOfArray` constructed from multi-dimensional arrays | ||
```julia | ||
VectorOfArray(u::AbstractArray{AT}) where {T, N, AT <: AbstractArray{T, N}} | ||
``` | ||
|
||
where `IndexStyle(typeof(u)) isa IndexLinear`. | ||
""" | ||
mutable struct VectorOfArray{T, N, A} <: AbstractVectorOfArray{T, N, A} | ||
u::A # A <: AbstractVector{<: AbstractArray{T, N - 1}} | ||
u::A # A <: AbstractArray{<: AbstractArray{T, N - 1}} | ||
end | ||
# VectorOfArray with an added series for time | ||
|
||
|
@@ -719,7 +718,7 @@ | |
# for VectorOfArray with multi-dimensional parent arrays of arrays where all elements are the same type | ||
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))) | ||
end | ||
|
||
# special-case when the multi-dimensional parent array is just an AbstractVector (call the old method) | ||
|
@@ -728,6 +727,7 @@ | |
return Base.similar(vec, eltype(vec)) | ||
end | ||
|
||
|
||
# fill! | ||
# For DiffEqArray it ignores ts and fills only u | ||
function Base.fill!(VA::AbstractVectorOfArray, x) | ||
|
@@ -840,12 +840,37 @@ | |
# make vectorofarrays broadcastable so they aren't collected | ||
Broadcast.broadcastable(x::AbstractVectorOfArray) = x | ||
|
||
# recurse through broadcast arguments and return a parent array for | ||
# the first VoA or DiffEqArray in the bc arguments | ||
function find_VoA_parent(args) | ||
arg = Base.first(args) | ||
if arg isa AbstractDiffEqArray | ||
# if first(args) is a DiffEqArray, use the underlying | ||
# field `u` of DiffEqArray as a parent array. | ||
return arg.u | ||
elseif arg isa AbstractVectorOfArray | ||
return parent(arg) | ||
else | ||
return find_VoA_parent(Base.tail(args)) | ||
end | ||
end | ||
|
||
@inline function Base.copy(bc::Broadcast.Broadcasted{<:VectorOfArrayStyle}) | ||
bc = Broadcast.flatten(bc) | ||
N = narrays(bc) | ||
VectorOfArray(map(1:N) do i | ||
copy(unpack_voa(bc, i)) | ||
end) | ||
|
||
parent = find_VoA_parent(bc.args) | ||
|
||
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 | ||
Comment on lines
+863
to
+873
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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). |
||
end | ||
|
||
for (type, N_expr) in [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broadcasting
similar
here avoids some#undef
issues.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 structureVector{AbstractArray{T}}
, so it shouldn't be a valid edge case. Maybe I'm missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Numbers are also allowed. That's used a lot downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can special-case that case where the elements are numbers, but we cannot throw it out.