Skip to content

Conversation

jlchan
Copy link
Contributor

@jlchan jlchan commented Apr 19, 2021

Enables both StructArrays.components and broadcasting with StaticArray types. Fixes #179.

@piever
Copy link
Collaborator

piever commented Apr 20, 2021

Thank you for the PR! It mostly looks good, I've added a few nitpicking in the comments.

jlchan added 2 commits April 24, 2021 23:28
enables non-allocating views for StaticArray types
@jlchan jlchan changed the title Adding support for components/broadcasting with StructArrays of StaticArrays Adding support for components/broadcasting/views with StructArrays of StaticArrays Apr 25, 2021
@jlchan
Copy link
Contributor Author

jlchan commented Apr 25, 2021

added @inline in e982f1a so that Base.view is non-allocating for StructArrays with tuple and StaticArray element types

Improves performance for tuples

julia> x = StructArray((rand(2,2),rand(2,2)));

julia> function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  935.464 ns (11 allocations: 416 bytes)

julia> @inline function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  3.906 ns (0 allocations: 0 bytes)

Improves performance for StaticArrays

julia> x = StructArray(SVector{2}.(rand(2,2), rand(2,2)));

julia> function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  932.808 ns (11 allocations: 416 bytes)

julia> @inline function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  4.092 ns (0 allocations: 0 bytes)

Doesn't seem to really impact NamedTuples.

julia> x = StructArray((a=rand(2,2),b=rand(2,2)));

julia> function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  201.029 ns (5 allocations: 304 bytes)

julia> @inline function Base.view(s::StructArray{T, N, C}, I...) where {T, N, C}
           StructArray{T}(map(v -> view(v, I...), StructArrays.components(s)))
       end

julia> @btime view($x,$:,$1);
  200.084 ns (5 allocations: 304 bytes)

@jlchan jlchan requested a review from piever April 25, 2021 04:36
@piever
Copy link
Collaborator

piever commented Apr 25, 2021

LGTM! I've suggested some minor changes of wording and formatting. If you are happy with those, you can just accept the changes and this should be good to go. I will just wait a little bit before merging because it is a breaking change, so that gives a chance to people relying on the old behavior to look at it.

jlchan and others added 3 commits April 25, 2021 09:26
@jlchan
Copy link
Contributor Author

jlchan commented Apr 25, 2021

Sounds great. Thanks for the guidance throughout this PR!

jlchan added a commit to jlchan/Trixi.jl that referenced this pull request Apr 30, 2021
@piever
Copy link
Collaborator

piever commented May 1, 2021

Thanks again for the very nice contribution!

@piever piever merged commit c91e285 into JuliaArrays:master May 1, 2021
@jlchan
Copy link
Contributor Author

jlchan commented May 1, 2021

Thank you for taking the time to review my code and walk me through my first PR!

@jlchan jlchan deleted the structarray_broadcasting branch May 1, 2021 15:57
@jlchan
Copy link
Contributor Author

jlchan commented Jun 23, 2021

@piever do have a timeline for releasing a new version with this PR? We're using the StaticArrays functionality in https://github.com/trixi-framework/Trixi.jl/, but the CI is somewhat involved and is having issues with rev="master".

@piever
Copy link
Collaborator

piever commented Jun 23, 2021

I was hoping to clump more breaking changes (from https://github.com/JuliaArrays/StructArrays.jl/milestone/2) with this one, but I clearly don't have the bandwidth to work on those at the moment. Even though this is the only breaking change, I guess it is not too bad to just tag 0.6 now (the updating should be almost automated for most dependent packages).

@jlchan
Copy link
Contributor Author

jlchan commented Jun 23, 2021

No pressure either way - we can work on the CI on our end too. However, a 0.6 tag would certainly be nice for our CI setup :)

@piever
Copy link
Collaborator

piever commented Jun 23, 2021

In the end, it's probably best to tag now to see if overall users are happy with the new StaticArrays support. I'll just wait a little bit to see if there is an easy solution to JuliaData/WeakRefStrings.jl#72 (to keep supporting julia 1.0), otherwise I'll just require julia 1.3 and tag.

@piever
Copy link
Collaborator

piever commented Jun 27, 2021

@jlchan the 0.6 tag was just merged in the General Registry.

@jlchan
Copy link
Contributor Author

jlchan commented Jun 27, 2021

Thank you! Much appreciated.

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.

Cannot both broadcast and access components with StructArrays of StaticArrays

2 participants