Idempotent indexing in range[IdOffsetRange]

[jishnub]

After this PR:

julia> r = OffsetArrays.IdOffsetRange(3:5, -1)
OffsetArrays.IdOffsetRange(2:4)

julia> (1:4)[r]
2:4 with indices 0:2

julia> axes((1:4)[r]) == axes(r)
true

On master:

julia> (1:4)[r]
2:4

As a consequence,

on master:

julia> a = zeros(2:4);

julia> r = 1:4;

julia> a[axes(a,1)] .= r[axes(a,1)]
ERROR: DimensionMismatch("array could not be broadcast to match destination")

after this PR:

julia> a = zeros(2:4);

julia> r = 1:4;

julia> a[axes(a,1)] .= r[axes(a,1)]
3-element view(OffsetArray(::Array{Float64,1}, 2:4), 2:4) with eltype Float64 with indices 2:4:
 2.0
 3.0
 4.0

[codecov]

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #161   +/-   ##
=======================================
  Coverage   99.21%   99.21%           
=======================================
  Files           4        4           
  Lines         256      256           
=======================================
  Hits          254      254           
  Misses          2        2           

Impacted Files	Coverage Δ
src/utils.jl	100.00% <ø> (ø)
src/OffsetArrays.jl	99.43% <100.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 87666ee...1783826. Read the comment docs.

[jishnub]

bump

[johnnychen94]

This looks really nice a fix to me but I'd still want to invite @timholy to double-check it.

[timholy]

It's a bit suspicious that the bodies of all the new methods are identical. Can you instead modify the generic AbstractRange methods above to achieve the same thing?

[jishnub]

It appears to be a bit difficult to achieve this in general without introducing ambiguities, as some of the range types have specific indexing behaviors defined in Base. Eg:

function getindex(r::StepRange, s::AbstractRange{<:Integer})

and

function getindex(r::StepRangeLen{T,<:TwicePrecision,<:TwicePrecision}, s::OrdinalRange{<:Integer}) where T

presumably indicate that we should specialize on the second argument?

[timholy]

Ah, yeah, good point. At some point we probably want to have some tests for this kind of thing that introduce a novel range object (could use https://github.com/JuliaArrays/CustomUnitRanges.jl to make that easy), just to make sure we go as far as we can with generic operations.

[timholy]

I have no other objections, though. This clearly improves the handling of range values and range indices 👍. So merge at will.

[jishnub]

Yes indeed. The present approach is patchwork at best, and a new range type introduced will not support idempotent indexing with an IdOffsetRange. I wonder what's the best approach to address this?

For example:

julia> struct MyUnitRange <: AbstractUnitRange{Int}
       start :: Int
       stop :: Int
       end

julia> Base.first(r::MyUnitRange) = r.start

julia> Base.last(r::MyUnitRange) = r.stop

julia> s = MyUnitRange(2,3);

julia> r = OffsetArrays.IdOffsetRange(4:5, -3)
OffsetArrays.IdOffsetRange(1:2)

julia> axes(r)
(OffsetArrays.IdOffsetRange(-2:-1),)

julia> s[r]
2:3

julia> axes(s[r])
(Base.OneTo(2),)

Ideally we should be able to define

getindex(r::AbstractRange, inds::IdOffsetRange)

however the problem with this is that a type that defines

getindex(r::CustomRange, inds::AbstractUnitRange{<:Integer})

will run into ambiguities.

[johnnychen94]

I'm still a little confused about how things are working under the hood, but tests look good. Very concise a patch, I agree that we need tests to make sure things don't get break unexpectedly.

[timholy]

I suspect it's a sign we may need better "inner methods" and implement the core range algorithms with them...although ranges are already pretty close to the metal. I suspect this is a multistage process, where we get this working and then rethink range implementation in Base.

[jishnub]

I've added methods that return an AbstractUnitRange instead of an OffsetArray when a UnitRange{<:Integer} is indexed with an IdOffstRange or an IdentityUnitRange. This is because the result may be further used in indexing, and indexing with ranges is faster than that with vectors.

before

julia> a = collect(1:10);

julia> ur = 3:6;

julia> idr = Base.IdentityUnitRange(2:4);

julia> @btime $a[$ur[$idr]];
  98.796 ns (1 allocation: 112 bytes)

julia> idor = OffsetArrays.IdOffsetRange(1:3, 1);

julia> @btime $a[$ur[$idor]];
  102.616 ns (1 allocation: 112 bytes)

after

julia> @btime $a[$ur[$idr]];
  76.405 ns (1 allocation: 112 bytes)

julia> @btime $a[$ur[$idor]];
  84.349 ns (1 allocation: 112 bytes)

[timholy]

This is great work, thanks @jishnub!

[johnnychen94]

A new patch release, maybe?