Fix colon-reshaping of OffsetVector

[timholy]

Fixes #33614

[rapus95]

Will this come to 1.0 LTS?

[timholy]

If someone adds a 1.0 backport. Let's see what reviewers think.

[ghost]

This is the result of this change with OffsetArrays.jl before JuliaArrays/OffsetArrays.jl#84:

julia> reshape(OffsetArray(-1:0, -1:0), :)
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing at ./abstractarray.jl:89 [inlined]
 [2] _reshape at ./reshapedarray.jl:167 [inlined]
 [3] reshape(::OffsetArray{Int64,1,UnitRange{Int64}}, ::Tuple{Colon}) at ./reshapedarray.jl:117
 [4] reshape(::OffsetArray{Int64,1,UnitRange{Int64}}, ::Colon) at ./reshapedarray.jl:116
 [5] top-level scope at REPL[5]:1

It seems unexpected. Maybe require_one_based_indexing in l.167 of reshapedarray.jl should be removed.

After JuliaArrays/OffsetArrays.jl#84 this problem doesn't occur.

[timholy]

You're right that reshape(A, :) should work when A::AbstractVector (and just be a no-op). I added a commit fixing this.

However, in general reshape(A, 2) should not, because reshape(a, 2) seems to imply that the result should have Base.OneTo indexing (or equivalent). reshape(a, ax) would be the syntax for specifying the actual axes, and reshape(a, :) implies preserving whatever axes a has currently.

In general Base has no way of knowing how to change the indexing of A. Consequently the require_one_based_indexing check still seems to make sense. Let's try two different array types that can have their indexing start at 0:

julia> using CatIndices, OffsetArrays

julia> a = OffsetArray([1.0, 2.0], 0:1)
2-element OffsetArray(::Array{Float64,1}, 0:1) with eltype Float64 with indices 0:1:
 1.0
 2.0

julia> b = BidirectionalVector([2.0])
1-element BidirectionalVector{Float64} with indices CatIndices.URange(1,1):
 2.0

julia> pushfirst!(b, 1.0)
2-element BidirectionalVector{Float64} with indices CatIndices.URange(0,1):
 1.0
 2.0

julia> axes(a)
(Base.IdentityUnitRange(0:1),)

julia> axes(b)
(CatIndices.URange(0,1),)

julia> reshape(a, :)
2-element OffsetArray(::Array{Float64,1}, 0:1) with eltype Float64 with indices 0:1:
 1.0
 2.0

julia> reshape(b, :)
2-element BidirectionalVector{Float64} with indices CatIndices.URange(0,1):
 1.0
 2.0

julia> reshape(a, 2)
2-element Array{Float64,1}:
 1.0
 2.0

julia> reshape(b, 2)
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing at ./abstractarray.jl:89 [inlined]
 [2] _reshape at ./reshapedarray.jl:168 [inlined]
 [3] reshape at ./reshapedarray.jl:112 [inlined]
 [4] reshape(::BidirectionalVector{Float64}, ::Int64) at ./reshapedarray.jl:116
 [5] top-level scope at REPL[17]:1

That's fine because CatIndices hasn't (yet) implemented a specialization of reshape.

And now let's combine them:

julia> c = OffsetArray(b, -5)
2-element OffsetArray(::BidirectionalVector{Float64}, -5:-4) with eltype Float64 with indices -5:-4:
 1.0
 2.0

julia> reshape(c, :)
2-element OffsetArray(::BidirectionalVector{Float64}, -5:-4) with eltype Float64 with indices -5:-4:
 1.0
 2.0

julia> reshape(c, 2)
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing at ./abstractarray.jl:89 [inlined]
 [2] _reshape at ./reshapedarray.jl:168 [inlined]
 [3] reshape at ./reshapedarray.jl:112 [inlined]
 [4] reshape at /home/tim/.julia/packages/OffsetArrays/vIbpP/src/OffsetArrays.jl:113 [inlined]
 [5] reshape(::OffsetArray{Float64,1,BidirectionalVector{Float64}}, ::Int64) at ./reshapedarray.jl:116
 [6] top-level scope at REPL[20]:1

This seems to be the right behavior, AFAICT.

[ghost]

The behavior about the colon seems to be inconsistent with the result for n-dim array.

julia> using OffsetArrays

julia> A = OffsetArray([1 2; 3 4], -1, -1)
2×2 OffsetArray(::Array{Int64,2}, 0:1, 0:1) with eltype Int64 with indices 0:1×0:1:
 1  2
 3  4

julia> B = OffsetArray([1, 2], -1)
2-element OffsetArray(::Array{Int64,1}, 0:1) with eltype Int64 with indices 0:1:
 1
 2

julia> axes(reshape(A, 2, :))
(Base.OneTo(2), Base.OneTo(2))

julia> axes(reshape(B, :))
(Base.IdentityUnitRange(0:1),)

In 2-dim array, reshape(A, 2, :) doesn't preserve axes. I think axes(reshape(B, :)) should be (Base.OneTo(2), ) for consistency.

In general Base has no way of knowing how to change the indexing of A

My understanding is that function reshape() overwrites the axes, not changes the existing ones, so it's not necessary to know the way to change. In the last line of your results, I think reshape(c, 2) should return the standard 2-element array.

[timholy]

The difference between 1- and higher dimensions is documented here. In particular, the part about linear indexing is relevant; reshaping is essentially an appeal to the linear index representation, and that differs for 1-dimensional objects (for which the linear index can start anywhere) and higher-dimensional objects (for which linear indexing always starts at 1).

My understanding is that function reshape() overwrites the axes, not changes the existing ones, so it's not necessary to know the way to change. In the last line of your results, I think reshape(c, 2) should return the standard 2-element array.

I guess that's not unreasonable. I can't really think about it carefully now but will try to do so over the weekend.

[KristofferC]

I guess this caused OffsetArrays to start failing?

Test Failed at /home/pkgeval/.julia/packages/OffsetArrays/XDZ8E/test/runtests.jl:7
  Expression: isempty(detect_ambiguities(OffsetArrays, Base, Core))
   Evaluated: isempty(Tuple{Method,Method}[(reshape(A::OffsetArray, inds::Union{Colon, Int64}...) in OffsetArrays at /home/pkgeval/.julia/packages/OffsetArrays/XDZ8E/src/OffsetArrays.jl:115, reshape(parent::AbstractArray{T,1} where T, ::Colon) in Base at reshapedarray.jl:115)])
ERROR: LoadError: There was an error during testing
in expression starting at /home/pkgeval/.julia/packages/OffsetArrays/XDZ8E/test/runtests.jl:7
ERROR: Package OffsetArrays errored during testing

cc @timholy

[timholy]

Fixed in OffsetArrays v0.11.3