Skip to content
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

Views of SArray may use getindex to avoid the SubArray wrapper #908

Merged
merged 7 commits into from
May 12, 2021

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented May 7, 2021

Since SArrays are immutable and support allocation-less vector indexing for certain index types, views of SArrays constructed with such indices do not need to retain the SubArray wrapper and may just return another SArray obtained through indexing. Fixes #892.

After this PR:

julia> s = SMatrix{3,3}(ones(3,3));

julia> sv = @view s[:, :]
3×3 SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 1.0  1.0  1.0
 1.0  1.0  1.0
 1.0  1.0  1.0

julia> @btime $(Ref(sv))[] + $(Ref(sv))[]
  3.730 ns (0 allocations: 0 bytes)
3×3 SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 2.0  2.0  2.0
 2.0  2.0  2.0
 2.0  2.0  2.0

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer not giving special treatment to SArray - if there were a generic way of doing this (like a trait for immutable arrays) that would be preferable.

On the other hand this is quite practical and seems harmless to merge.

test/SArray.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great now, could you just bump version to 1.2.0? I'll merge and tag a new release.

@jishnub
Copy link
Member Author

jishnub commented May 12, 2021

Rebased on master and bumped version to 1.2.0

@KristofferC
Copy link
Contributor

KristofferC commented Oct 2, 2021

This PR caused a difference between using a Matrix and a SMatrix in ForwardDiff as described in JuliaDiff/ForwardDiff.jl#547 (comment). I bisected to this PR with the following script:

using ForwardDiff, LinearAlgebra, StaticArrays


foo2(A) = SVector{4}(norm(r, 2) for r = eachrow(A))

function foo1!(out, x; μ = 0.8 / 2)
    λ = SVector{3}(@view x[1:3])
    K = SMatrix{3,3}(@view x[4:12])

    A_λ = [ 1   0  -1   0 ;
            0   1   0  -1 ;
            μ   μ   μ   μ ]

    out[1:4] = A_λ' * λ + foo2(A_λ' * K)

    out
end

function foo1_s!(out, x; μ = 0.8 / 2)
    λ = SVector{3}(@view x[1:3])
    K = SMatrix{3,3}(@view x[4:12])

    A_λ = @SMatrix [ 1   0  -1   0 ;
                     0   1   0  -1 ;
                     μ   μ   μ   μ ]

    out[1:4] = A_λ' * λ + foo2(A_λ' * K)

    out
end

x = zeros(12);
out = zeros(4);
display(ForwardDiff.jacobian(foo1!, out, x))
display(ForwardDiff.jacobian(foo1_s!, out, x))

nothing

and before this PR they returned the same value, after it, the one with @SMatrix only has NaNs in it.

@mateuszbaran
Copy link
Collaborator

The difference is in one check that LinearAlgebra's norm2 does which isn't done by StaticArrays.jl:

https://github.com/JuliaLang/julia/blob/a9a1d80076f98b1e31197c22924acf24bf5da760/stdlib/LinearAlgebra/src/generic.jl#L498

Probably StaticArrays.jl needs to have this check too?

@ferrolho
Copy link

ferrolho commented Oct 4, 2021

Thank you for finding this PR as the breaking changes, @KristofferC. And thank you @mateuszbaran for your reply.

Should I create a new issue to track this?

@KristofferC
Copy link
Contributor

Sounds like a good idea.

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.

Allocations in broadcasting with views of StaticArrays
5 participants