Reshape with an offset parent array

[jishnub]

Fixes #219

Now

julia> r = OffsetArray(IdentityUnitRange(0:1), -1)
Base.IdentityUnitRange(0:1) with indices -1:0

julia> reshape(r, 2) # Int arguments strip offsets
0:1

julia> reshape(r, (2,))
0:1

julia> reshape(r, :) # Colon preserves offsets for an OffsetVector
Base.IdentityUnitRange(0:1) with indices -1:0

julia> reshape(r, (:,))
Base.IdentityUnitRange(0:1) with indices -1:0

The last one is a bugfix, as on master

julia> reshape(r, :)
Base.IdentityUnitRange(0:1) with indices -1:0

julia> reshape(r, (:,))
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:

[codecov]

Codecov Report

Merging #245 (4aef07c) into master (7a43b4c) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   95.26%   95.41%   +0.15%     
==========================================
  Files           5        5              
  Lines         422      436      +14     
==========================================
+ Hits          402      416      +14     
  Misses         20       20              

Impacted Files	Coverage Δ
src/OffsetArrays.jl	98.29% <100.00%> (+0.06%)	⬆️
src/utils.jl	97.95% <100.00%> (+0.23%)	⬆️
src/axes.jl	100.00% <0.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 7a43b4c...4aef07c. Read the comment docs.

[johnnychen94]

julia> reshape(r, :) # Colon preserves offsets for an OffsetVector
Base.IdentityUnitRange(0:1) with indices -1:0

On whether Colon should preserve the offset, looks like there's a back&forth here. #228 (comment)

I still believe we should preserve the offset because it provides more information. But that's definitely breaking, so need to properly deprecate it before v2.0.

[jishnub]

This is actually the present behavior (a single colon preserves offsets for an OffsetVector), and this PR doesn't change that.

The present behavior (on master) is:

julia> r = ones(2:3);

julia> reshape(r, :) # A single colon preserves offsets for an OffsetVector
2-element OffsetArray(::Vector{Float64}, 2:3) with eltype Float64 with indices 2:3:
 1.0
 1.0

julia> reshape(ones(2:3, 2:3), :) # offsets are stripped for an OffsetArray other than an OffsetVector
4-element Vector{Float64}:
 1.0
 1.0
 1.0
 1.0

julia> reshape(r, (2, :)) # colon with any other argument strips offsets
2×1 Matrix{Float64}:
 1.0
 1.0

julia> reshape(ones(2:3, 2:3), (3:4, :))
2×2 OffsetArray(::Matrix{Float64}, 3:4, 1:2) with eltype Float64 with indices 3:4×1:2:
 1.0  1.0
 1.0  1.0

In that regard I think #228 is the right step, where a Colon preserves offsets if possible. This PR preserves the present behavior and is not breaking.

[johnnychen94]

The last round of review with only some style suggestions. Feel free to merge when you're ready.

[johnnychen94]

Is this comment still useful? I guess no?

[jishnub]

@johnnychen94 I've added some bugfixes, although the fix seems a bit hacky. Could you take a look? The main changes are in 9f91760, where I add _reshape2 to deal with cases where no_offset_view applied to an OffsetArray returns another OffsetArray, which leads to stackoverflows. Now sure what's the best thing to do here, currently I just push the reshape to the parent and hope for the best.

[johnnychen94]

I can basically get your points here. And yes it is also tricky for me.

There might be some deeply hidden bugs but nothing suspicious to me so far.