jw3126 / Setfield.jl

Update deeply nested immutable structs.
Other
167 stars 17 forks source link

Setting OffsetArray(::StaticArray) falling back to Vector #153

Open marius311 opened 3 years ago

marius311 commented 3 years ago

@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

jw3126 commented 3 years ago

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

marius311 commented 3 years ago

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.

jw3126 commented 3 years ago

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