Fix nested IdOffsetRange constructor

[jishnub]

There was a bug introduced by #178 in defining IdOffsetRange(::IdOffsetRange, offset), this PR fixes it.

On master

julia> b = 1:10
1:10

julia> bov = OffsetArray(view(b, 3:4), 3:4);

julia> c = @view b[bov]
2-element view(::UnitRange{Int64}, 3:4 with indices 3:4) with eltype Int64 with indices 3:4:
 3
 4

julia> d = OffsetArray(c, 1:2)
2-element OffsetArray(view(::UnitRange{Int64}, 3:4 with indices 3:4), 1:2) with eltype Int64 with indices 1:2:
 1
 2

After this PR:

julia> d = OffsetArray(c, 1:2)
2-element OffsetArray(view(::UnitRange{Int64}, 3:4 with indices 3:4), 1:2) with eltype Int64 with indices 1:2:
 3
 4

[codecov]

Codecov Report

Merging #185 (7f4eecb) into master (94a4172) will increase coverage by 0.36%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   98.91%   99.28%   +0.36%     
==========================================
  Files           5        5              
  Lines         277      280       +3     
==========================================
+ Hits          274      278       +4     
+ Misses          3        2       -1     

Impacted Files	Coverage Δ
src/utils.jl	100.00% <ø> (ø)
src/axes.jl	100.00% <100.00%> (+2.08%)	⬆️
src/OffsetArrays.jl	98.93% <0.00%> (+0.01%)	⬆️

---

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 94a4172...7f4eecb. Read the comment docs.

[jishnub]

Fixes #186 , this was a bug in OffsetArrays after all. Now

julia> a = reshape(1:12, 3, 4)
3×4 reshape(::UnitRange{Int64}, 3, 4) with eltype Int64:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> view(a, :, OffsetArrays.IdOffsetRange(3:4)) == view(a, :, 3:4)
true

julia> vo = view(a, OffsetArrays.IdOffsetRange(2:3), 3)
2-element view(reshape(::UnitRange{Int64}, 3, 4), OffsetArrays.IdOffsetRange(2:3), 3) with eltype Int64 with indices 1:2:
 8
 9

[timholy]

Do you need this? AFAICT it does the same thing as the default. If this is for ambiguity resolution, perhaps add a comment?

[jishnub]

This is to avoid a stack overflow error in another method. I'll leave a comment.

On master:

julia> IdOffsetRange{Int, IdOffsetRange{Int, UnitRange{Int}}}(IdOffsetRange(2:3, 1), 1)
ERROR: StackOverflowError:
Stacktrace:
 [1] IdOffsetRange at [...]/OffsetArrays.jl/src/axes.jl:80 [inlined]
 [2] IdOffsetRange at [...]/OffsetArrays.jl/src/axes.jl:85 [inlined]
 [3] convert at ./range.jl:148 [inlined]
 [4] offset_coerce at [...]/OffsetArrays.jl/src/axes.jl:125 [inlined]
 [5] IdOffsetRange{Int64,IdOffsetRange{Int64,UnitRange{Int64}}}(::IdOffsetRange{Int64,UnitRange{Int64}}, ::Int64) at [...]/OffsetArrays.jl/src/axes.jl:98
 [6] IdOffsetRange{Int64,IdOffsetRange{Int64,UnitRange{Int64}}}(::IdOffsetRange{Int64,UnitRange{Int64}}, ::Int64) at [...]/OffsetArrays.jl/src/axes.jl:99 (repeats 79983 times)

This is fixed by this method.

The method called on master is IdOffsetRange{T,I}(::IdOffsetRange, offset::Integer) which tries to coerce the argument types, and call IdOffsetRange{T,I}(::I, ::T) within itself. This is presumably meant for cases like

julia> IdOffsetRange{Int, UnitRange{Int}}(IdOffsetRange(2:3, 1), 1)
OffsetArrays.IdOffsetRange(4:5)

however if the typevar I itself is an IdOffsetRange it ends up calling itself because the type signature is more specific. There might be a better way around this than adding this extra method, I'd just introduced this to avoid the overflow.

[timholy]

Makes sense. Perhaps the argument-type coercion doesn't need to specialize on IdOffsetRange for the range? If that's not an easy fix, then just adding a brief comment would be a nice addition.

[jishnub]

I think the idea behind that method was to remove the IdOffsetRange wrapper and coerce the parent range, as there is another method for generic AbstractUnitRange arguments. Not quite sure of the performance impacts of removing this, perhaps one may look into it in another PR. As of now I have added a comment in 7f4eecb

[timholy]

Very good!

Would this have been easier to detect and analyze if IdOffsetRange printed the indices as well as values? I understand that not everyone likes #179, but should we change the printing some other way so that it is more revealing?

[jishnub]

Yes indeed, halfway through debugging this I was wishing that the indices were printed along with the values. It'll be good if we can figure out a way to display them.