Rely on the fallback definition of checkindex

[timholy]

Tests pass without it, and it's a source of ambiguities with FFTViews.

[codecov]

Codecov Report

Merging #255 (0e24bf7) into master (96fd74e) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
- Coverage   95.41%   95.40%   -0.02%     
==========================================
  Files           5        5              
  Lines         436      435       -1     
==========================================
- Hits          416      415       -1     
  Misses         20       20              

Impacted Files	Coverage Δ
src/OffsetArrays.jl	98.28% <ø> (-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 96fd74e...0e24bf7. Read the comment docs.

[jishnub]

This was a performance enhancement, as otherwise bounds-checking does not realize that an OffsetRange is essentially a range, and loops over each element.

Master

julia> r = OffsetArray(1:1000);

julia> @btime checkbounds(Bool, 1:10000, $(Ref(r))[]);
  3.207 ns (0 allocations: 0 bytes)

This branch

julia> @btime checkbounds(Bool, 1:10000, $(Ref(r))[]);
  1.604 μs (0 allocations: 0 bytes)

Wonder what's the best way to avoid such ambiguities are, as they are bound to crop up when one package specializes on the first argument and another on the second.

[timholy]

Wonder what's the best way to avoid such ambiguities are, as they are bound to crop up when one package specializes on the first argument and another on the second.

Indirection/traits. We need to fix checkbounds so that it relies on an internal method to make the decision about whether it's a range. E.g.,

abstract type RangeStyle end
struct IsRange <: RangeStyle end
struct NotARange <: RangeStyle end
RangeStyle(::AbstractVector) = NotARange()

checkindex(::Type{Bool}, inds::AbstractUnitRange, index::AbstractVector) = _checkindex(Bool, inds, RangeStyle(index), index)
_checkindex(Bool, inds, ::IsRange, index) = checkindex(Bool, inds, first(index) & checkindex(Bool, inds, last(index))
_checkindex(Bool, inds, ::RangeStyle, index) = # the current AbstractArray fallback

Then this package could specialize RangeStyle for an OffsetRange.

Would you like to add that or should I? One should carefully consider names etc: do we really want a single trait that declares "this is a range for all purposes" or a narrower one that says "all you have to do is check the endpoints"?

We'll have to have a transitional plan until Julia 1.8 comes out, but let's get the right fix done first and then figure out the bandaid later.

[jishnub]

Could you add this to Base? That'll be great! I'm a bit caught up at the moment.

[timholy]

Sure! For when we get to the bandaid, can you give a real-world example where this performance matters? I.e., something that a normal user might actual trip across. One can of course use a IdOffsetRange for indexing purposes, and that is an AbstractUnitRange.

[jishnub]

Yes for indices that are OffsetArrays wrapping AbstractUnitRanges, we do presently convert them to IdOffsetRanges in the indexing operation. The performance matters when the parent indices are not AbstractUnitRanges, eg. offset StepRanges.

julia> r = 1:1:100;

julia> ro = OffsetArray(r);

julia> v = ones(1000);

julia> @btime $v[$r];
  237.578 ns (1 allocation: 896 bytes)

julia> @btime $v[$ro];
  5.158 μs (1 allocation: 896 bytes)

Interestingly even with bounds-checking disabled there's a performance hit, which might be improved.

julia> f(v, r) = @inbounds v[r];

julia> @btime f($v, $r);
  233.909 ns (1 allocation: 896 bytes)

julia> @btime f($v, $ro);
  2.642 μs (1 allocation: 896 bytes)

[timholy]

I guess we have to make a decision then:

• handle indexing decisions via traits
• handle indexing decisions by subtyping

If we go with the latter, then all the methods to index by an OffsetArray should go to the fallbacks and not be specialized; the potential for ambiguities is far too large. The best solution in that case will also be to create an OffsetStepRange and then just tell people that they should only be indexing with AbstractRanges if they want AbstractRange performance.

If we go with the former, we may have quite a lot of work to do!

[jishnub]

I'm in support of the trait-based approach, although I realize it's a lot of work. Suggesting an user to be conscious of types seems far from ideal. Also adding a new type would invariably lead to a lot of ambiguities. Moreover, other packages trying something similar would have to reinvent the wheel unless they study this package well. Ideally Base would handle these performance issues through traits.