Tweak getindex implementation + test corner cases

[willtebbutt]

Resolves #546

[willtebbutt]

Bump @MikeInnes

[MikeInnes]

Seems sensible from a quick look. LMK if there's anything in particular you wanted me to check for.

[willtebbutt]

Nope, I'll merge if you approve.

[CarloLucibello]

Seems sensible from a quick look. LMK if there's anything in particular you wanted me to check for.

@willtebbutt that sounds as an approval to me :) we can merge if you are ok

[willtebbutt]

bors r+

[bors]

Build succeeded:

• ci/gitlab/gitlab.com

[marius311]

I started getting the error attached below in the tests for my package which uses Zygote+StaticArrays. My StaticArrays version has not changed but I bisected the problem to down to the b3d0ef7 commit on the Zygote side in this PR.

I'm totally unfamiliar with what this PR does and also with what that StaticArrays error means. I can say that my package uses StaticArrays{<:MyCustomArrayImplementation} and I have also defined custom adjoints for some operations on MyCustomArrayImplementation, if that helps at all.

Any guidance you guys might offer on how to figure out if this is a problem in my package, in Zygote, or in StaticArrays?

Thanks.

Error During Test at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:345
  Test threw exception
  Expression: (gradient((f->((Map(∇[1] * f))' * Map(v[1]) + (Map(∇[2] * f))' * Map(v[2]);)), f))[1] ≈ ∇' * v
  setindex!() with non-isbitstype eltype is not supported by StaticArrays. Consider using SizedArray.
  Stacktrace:
   [1] error(::String) at ./error.jl:33
   [2] setindex! at /home/travis/.julia/packages/StaticArrays/1g9bq/src/MArray.jl:109 [inlined]
   [3] macro expansion at /home/travis/.julia/packages/StaticArrays/1g9bq/src/arraymath.jl:105 [inlined]
   [4] _fill! at /home/travis/.julia/packages/StaticArrays/1g9bq/src/arraymath.jl:101 [inlined]
   [5] fill!(::StaticArrays.MArray{Tuple{2},Union{Nothing, FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}}},1,2}, ::Nothing) at /home/travis/.julia/packages/StaticArrays/1g9bq/src/arraymath.jl:99
   [6] _zero(::StaticArrays.SArray{Tuple{2},FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}},1,2}, ::Type{T} where T) at /home/travis/.julia/packages/Zygote/fOyJ9/src/lib/array.jl:52
   [7] (::Zygote.var"#1035#1037"{StaticArrays.SArray{Tuple{2},FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}},1,2},Tuple{Int64}})(::FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}}) at /home/travis/.julia/packages/Zygote/fOyJ9/src/lib/array.jl:40
   [8] (::Zygote.var"#2730#back#1031"{Zygote.var"#1035#1037"{StaticArrays.SArray{Tuple{2},FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}},1,2},Tuple{Int64}}})(::FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}}) at /home/travis/.julia/packages/ZygoteRules/6nssF/src/adjoint.jl:49
   [9] literal_getindex at /home/travis/.julia/packages/Zygote/fOyJ9/src/lib/lib.jl:94 [inlined]
   [10] (::typeof(���(literal_getindex)))(::FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}}) at /home/travis/.julia/packages/Zygote/fOyJ9/src/compiler/interface2.jl:0
   [11] #32 at ./none:0 [inlined]
   [12] (::typeof(∂(λ)))(::Float64) at /home/travis/.julia/packages/Zygote/fOyJ9/src/compiler/interface2.jl:0
   [13] (::Zygote.var"#36#37"{typeof(∂(λ))})(::Float64) at /home/travis/.julia/packages/Zygote/fOyJ9/src/compiler/interface.jl:36
   [14] gradient(::Function, ::FlatMap{2×2 map, 1′ pixels, fourier∂, Array{Float64}}) at /home/travis/.julia/packages/Zygote/fOyJ9/src/compiler/interface.jl:45
   [15] top-level scope at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:345
   [16] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
   [17] top-level scope at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:345
   [18] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
   [19] top-level scope at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:311
   [20] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
   [21] top-level scope at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:304
   [22] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
   [23] top-level scope at /home/travis/build/marius311/CMBLensing.jl/test/runtests.jl:22

[willtebbutt]

Hmm yeah, the current implementation assumes mutability of arrays. We could just special-case StaticArrays, but would be nice to have a more general solution. @MikeInnes any thoughts?

[MikeInnes]

It's a bit unfortunate that Base doesn't have any way to tell mutable from immutable arrays, since that leaves us no real choice but to special case something, or at least to add a new API that new arrays may have to hook into.

The best option short term is probably something like: define a setindex method which returns a potentially-new array. It can fall back to mutating and returning the input, and overload on StaticArrays to use that package's method.

[willtebbutt]

Okay. I've not got bandwidth to deal with this right now. @MikeInnes do you know if anyone is able to deal with this?

[marius311]

Dug into this some more, and as you can see in the linked issues here (#686) and in StaticArrays (JuliaArrays/StaticArrays.jl#799), my current thinking is that its actually a problem in StaticArrays, basically because the call to similar(::StaticArray, Union{T,Nothing}) that's happening here is returning you an unusable object.