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

similar(::StaticArray, T) when !isbitstype(T) should give a SizedArray #799

Closed
marius311 opened this issue Jun 15, 2020 · 3 comments · Fixed by #1196
Closed

similar(::StaticArray, T) when !isbitstype(T) should give a SizedArray #799

marius311 opened this issue Jun 15, 2020 · 3 comments · Fixed by #1196

Comments

@marius311
Copy link
Contributor

The similar(::StaticArray, T) method returns an MArray even if called on an SArray, rightfully so since there would be no reason to call similar if you couldn't mutate the array later.

However, if !isbitstype(T), then the returned MArray is still un-mutable later because of

if isbitstype(T)
GC.@preserve v unsafe_store!(Base.unsafe_convert(Ptr{T}, pointer_from_objref(v)), convert(T, val), i)
else
# This one is unsafe (#27)
# unsafe_store!(Base.unsafe_convert(Ptr{Ptr{Nothing}}, pointer_from_objref(v.data)), pointer_from_objref(val), i)
error("setindex!() with non-isbitstype eltype is not supported by StaticArrays. Consider using SizedArray.")

So I think the natural thing would be for similar(::StaticArray, T) to return a SizedArray when !isbitstype(T), as basically suggested by that error message.

If people here agree, I can take a stab at a PR, but also I'm happy if someone else that's more familiar with the code-base does it (probably much faster than I can).

FWIW, this would fix FluxML/Zygote.jl#686 where this came up for me.

@andyferris
Copy link
Member

It’s possible that SizedArray is competitive with MArray on Julia 1.5. If so, we should just switch it over.

@c42f
Copy link
Member

c42f commented Jun 15, 2020

Well I don't think SizedArray will be competitive wrt allocation of temporaries in the current compiler - I don't think the optimizer can stack allocate those. Ideally we would finish #749 but in a way which is actually efficient, given the changes in 1.5.

More immediately, we could dispatch on isbits(eltype(a)) to ensure we return something which is mutable and as efficient as possible within the constraints of the current compiler. That could be either SizedArray or MArray as necessary, with the goal of returning MArray as much as possible?

@jishnub
Copy link
Member

jishnub commented Dec 2, 2022

Gentle bump, this leads to failure such as

julia> S = Diagonal(SVector{2,Real}(1,2))
2×2 Diagonal{Real, SVector{2, Real}} with indices SOneTo(2)×SOneTo(2):
 1  
   2

julia> S[:, 1]
ERROR: setindex!() with non-isbitstype eltype is not supported by StaticArrays. Consider using SizedArray.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] setindex!
   @ ~/.julia/packages/StaticArrays/B0HhH/src/MArray.jl:39 [inlined]
 [3] macro expansion
   @ ./multidimensional.jl:903 [inlined]
 [4] macro expansion
   @ ./cartesian.jl:64 [inlined]
 [5] _unsafe_getindex!
   @ ./multidimensional.jl:898 [inlined]
 [6] _unsafe_getindex(::IndexCartesian, ::Diagonal{Real, SVector{2, Real}}, ::Base.Slice{SOneTo{2}}, ::Int64)
   @ Base ./multidimensional.jl:889
 [7] _getindex
   @ ./multidimensional.jl:875 [inlined]
 [8] getindex(::Diagonal{Real, SVector{2, Real}}, ::Function, ::Int64)
   @ Base ./abstractarray.jl:1241
 [9] top-level scope
   @ REPL[20]:1

It would be good to have a SizedArray here

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 a pull request may close this issue.

4 participants