Setfield.jl icon indicating copy to clipboard operation
Setfield.jl copied to clipboard

Setting OffsetArray(::StaticArray) falling back to Vector

Open marius311 opened this issue 3 years ago • 3 comments

@set! is awesome for StaticArrays,

julia> x = @SVector(zeros(Int,3))
3-element SVector{3, Int64} with indices SOneTo(3):
 0
 0
 0

julia> @set x[1] = 1
3-element SVector{3, Int64} with indices SOneTo(3):
 1
 0
 0

but if the array if OffsetArray(::StaticArray), it seems to fall back to OffsetArray(::Array).

julia> x = OffsetArray(@SVector(zeros(Int,3)),-1)
3-element OffsetArray(::SVector{3, Int64}, 0:2) with eltype Int64 with indices 0:2:
 0
 0
 0

julia> @set x[1] = 1
3-element OffsetArray(::Vector{Int64}, 0:2) with eltype Int64 with indices 0:2:
 0
 1
 0

Is there some special-case I could define in my session to handle this? Could this be fixed generally?

Julia 1.6 / Setfield v0.7.0 / StaticArrays v1.0.1 / OffsetArrays v1.5.3

marius311 avatar Feb 10 '21 23:02 marius311

Great issue! Ideally @set x[...] would just call Base.setindex(x, ...). However Base.setindex is only defined for very few containers. And we can't increase coverage here without piracy. The best way to solve this would be

  • Revive https://github.com/JuliaLang/julia/pull/33495 I think there is nobody really against it, it just fell asleep and now is probably abandoned. Is that correct @tkf ? Do you have time to shepherd https://github.com/JuliaLang/julia/pull/33495 @tkf? Or would you be happy if someone takes it and finishes it?
  • Do a PR to OffsetArrays, that overloads setindex(::OffsetArray, ...)
  • Delete the workarounds in this package or rather Accessors.jl.

jw3126 avatar Feb 11 '21 08:02 jw3126

Do a PR to OffsetArrays, that overloads setindex(::OffsetArray, ...)

I actually tried defining this locally, but Base.setindex(::OffsetArray) didnt work (if thats what you meant), however defining Setfield.setindex did work (and I suppose wouldn't be considered piracy?) Here's what I had for reference:

@inline function Setfield.setindex(A::OffsetVector{T,S}, val, i::Int) where {T,S<:StaticArray}
    @boundscheck checkbounds(A, i)
    OffsetVector{T,S}(setindex(parent(A), val, OffsetArrays.parentindex(Base.axes1(A), i)), A.offsets)
end

Anyway, you certainly know better than me, but thought I'd mention that.

marius311 avatar Feb 11 '21 08:02 marius311

Yes it is fine to do that. I will only break, if the issue is fixed properly.

jw3126 avatar Feb 11 '21 08:02 jw3126