Reshape accepts a single colon for AbstractArrays

[jishnub]

Currently this is broken as reshape uses _offset to compute the offsets, but that function doesn't accept Colons. This PR fixes this.

Now:

julia> A0 = [1 3; 2 4]
2×2 Array{Int64,2}:
 1  3
 2  4

julia> reshape(A0, 2:3, :)
2×2 OffsetArray(::Array{Int64,2}, 2:3, 1:2) with eltype Int64 with indices 2:3×1:2:
 1  3
 2  4

I'm not totally sure if the fix is correct, but reshape is pretty broken for non 1-based arrays to add more tests.

[codecov]

Codecov Report

Merging #220 (e8740a5) into master (ac9821e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files           5        5           
  Lines         301      301           
=======================================
  Hits          296      296           
  Misses          5        5           

Impacted Files	Coverage Δ
src/utils.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac9821e...e8740a5. Read the comment docs.

[johnnychen94]

When it comes to OffsetArray, the axes seems not correct to me:

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

julia> reshape(A1, 2:4, :)
3×2 OffsetArray(::Matrix{Int64}, 2:4, 1:2) with eltype Int64 with indices 2:4×1:2:
 1  5
 4  3
 2  6

julia> reshape(A1, 2:3, :)
2×3 OffsetArray(::Matrix{Int64}, 2:3, 1:3) with eltype Int64 with indices 2:3×1:3:
 1  2  3
 4  5  6

OffsetArrays.jl/src/OffsetArrays.jl

Lines 288 to 289 in b74ae55

	Base.reshape(A::OffsetArray, inds::Tuple{OffsetAxis,Vararg{OffsetAxis}}) =
	OffsetArray(reshape(parent(A), map(_indexlength, inds)), map(_indexoffset, inds))

I think the problem here is that we strip the OffsetArray wrapper, and thus loss the : information. Hence we might need to manually construct the correct axes of : by using A instead of parent(A) here.

[johnnychen94]

We can let OffsetArray constructor decide how to deal with Colon:

Suggested change

	_offset(axparent::AbstractUnitRange, ::Union{Integer, Colon}) = 1 - first(axparent)
	_offset(axparent::AbstractUnitRange, ::Integer) = 1 - first(axparent)
	_offset(axparent::AbstractUnitRange, ::Colon) = Colon()

[jishnub]

We may do this, but this would mean interpreting Colon as an offset as opposed to an axis as we do now. So after this a Tuple{Vararg{Union{Integer, Colon}}} may be seen as offsets and other combinations (eg. Colons and AbstractUnitRanges, CartesianIndices etc) as axes. I think it should be clear from the context what the meaning of the colon is.

[jishnub]

I'm not sure what the correct choice is here. Eg. in the first example, reshape(A1, 2:4, :) can not preserve the axes of A1, as the number of elements along the second axis changes. Currently reshape only preserves the number of elements, and does not try to preserve the axes. In that sense the Colon may be interpreted as a placeholder for an Integer, so reshape(A2, 2:3, :) is equivalent to reshape(A2, 2:3, 3), and reshape(A2, 2:4, :) is equivalent to reshape(A2, 2:4, 2)

[johnnychen94]

I don't think we have ever interpreted Colon as an integer(length) instead of a range (axes) in Julia.

From the docstring of Colon:

Colons (:) are used to signify indexing entire objects or dimensions at once.
Very few operations are defined on Colons directly; instead they are converted by to_indices to an internal vector type (Base.Slice) to represent the collection of indices they span before being used.

[jishnub]

I meant that _offset(axparent::AbstractUnitRange, ::Colon) = Colon() would lead to this interpretation, as the result of _offset is interpreted as an Integer offset passed to the OffsetArray constructor. Perhaps we may use a different function in reshape instead of _offset

[johnnychen94]

Perhaps we may use a different function in reshape instead of _offset

I give it an implementation in #228

[johnnychen94]

Since this one only extends the current behavior, I'm good with it if you want to have it merged. In the meantime, we can keep #228 an 2.0 issue.