Inconsistencies in `setindex!`, and thus `vcat`

[mcabbott]

It would be nice if these all worked:

julia> reduce(vcat, [OffsetArray([i; 10i;;], 3,4) for i in 1:2])
4×1 Matrix{Int64}:
  1
 10
  2
 20

julia> ans == vcat([OffsetArray([i; 10i;;], 3,4) for i in 1:2]...)
true

julia> reduce(vcat, [OffsetArray([i,10i],3) for i in 1:2])
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing
   @ ./abstractarray.jl:127 [inlined]
 [2] setindex!
   @ ./array.jl:977 [inlined]
 [3] _typed_vcat!(a::Vector{Int64}, V::Vector{OffsetVector{Int64, Vector{Int64}}})
   @ Base ~/.julia/dev/julia/base/abstractarray.jl:1625

julia> vcat([OffsetArray([i,10i],3) for i in 1:2]...)
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1

Relatedly, writing an offset vector into a Matrix is fine, but into a Vector (as above) does not work. However, writing into a SubArray does work again:

julia> let ov = OffsetArray([1,2],3)
         x = rand(4,1)
         x[2:3, :] = ov
         x
       end
4×1 Matrix{Float64}:
 0.6150462970413711
 1.0
 2.0
 0.019942484431290763

julia> let ov = OffsetArray([1,2],3)
         x = rand(4)
         x[2:3] = ov  # writing into a vector it fails.
         x
       end
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing
   @ ./abstractarray.jl:127 [inlined]
 [2] setindex!(A::Vector{Float64}, X::OffsetVector{Int64, Vector{Int64}}, I::UnitRange{Int64})
   @ Base ./array.jl:977

julia> let ov = OffsetArray([1,2],3)
         x = rand(4)
         xv = view(x, :)  # not a Vector, a different path
         xv[2:3] = ov
         x
       end
4-element Vector{Float64}:
 0.009140994466162344
 1.0
 2.0
 0.5207867417714482

[jishnub]

We definitely need more consistency in how setindex! deals with offset indices. Currently we have this rather unsatisfactory situation:

julia> a = rand(4);

julia> v = OffsetArray(a, 2);

julia> a[:] = v
4-element OffsetArray(::Vector{Float64}, 3:6) with eltype Float64 with indices 3:6:
[...]

julia> a[:] .= v
ERROR: DimensionMismatch: array could not be broadcast to match destination
Stacktrace:
 [1] check_broadcast_shape
   @ ./broadcast.jl:540 [inlined]
 [2] check_broadcast_axes
   @ ./broadcast.jl:543 [inlined]
 [3] instantiate
   @ ./broadcast.jl:284 [inlined]
 [4] materialize!
   @ ./broadcast.jl:871 [inlined]
 [5] materialize!(dest::SubArray{Float64, 1, Vector{Float64}, Tuple{Base.Slice{Base.OneTo{Int64}}}, true}, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(identity), Tuple{OffsetVector{Float64, Vector{Float64}}}})
   @ Base.Broadcast ./broadcast.jl:868
 [6] top-level scope
   @ REPL[12]:1

julia> a[axes(a,1)] = v
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing
   @ ./abstractarray.jl:110 [inlined]
 [2] setindex!(A::Vector{Float64}, X::OffsetVector{Float64, Vector{Float64}}, I::Base.OneTo{Int64})
   @ Base ./array.jl:974
 [3] top-level scope
   @ REPL[13]:1

Perhaps we should require indices to match in a setindex! operation, and use copyto! when we don't care about the indices?

Related: https://github.com/JuliaLang/julia/issues/45374

[mcabbott]

Perhaps we should require indices to match in a setindex! operation, and use copyto! when we don't care

Something like this may have been the intention, copyto! iterates but setindex! indexes. But incompletely implemented?

However, making vcat work in all cases sounds like a good idea. And breaking cases where vcat now works sounds bad. It doesn't seem crazy to me to interpret setindex! as requiring that size match, but not axes.

Broadcasting requires axes, always, that's OK.

[jishnub]

Concatenation strips the axis information anyway, so perhaps it should not use setindex! internally, and rely on copyto! instead. What do you think?

[mcabbott]

Well, it as I said it does use setindex!. I think this is faster than copyto!, where it uses it. And it checks size where copyto! does not.

I was trying to figure out whether there was some quirk of linear-vs-cartesian indexing driving this, but I don't think so, I think it's just a bug.

[jishnub]

I guess the fix should go into Base? Is there something that needs to be changed in OffsetArrays?