Implement Base.checkindex for InvertedIndex

[nalimilan]

Is there any reason why checkindex(::Type{Bool}, ..., ::InvertedIndex) isn't implemented? CategoricalArrays currently doesn't work with Not because of this (JuliaData/CategoricalArrays.jl#296):

julia> using CategoricalArrays, InvertedIndices

julia> categorical(1:10)[Not([3])]
ERROR: ArgumentError: unable to check bounds for indices of type InvertedIndex{Array{Int64,1}}
Stacktrace:
 [1] checkindex(::Type{Bool}, ::Base.OneTo{Int64}, ::InvertedIndex{Array{Int64,1}}) at ./abstractarray.jl:561
 [2] checkbounds at ./abstractarray.jl:491 [inlined]
 [3] checkbounds at ./abstractarray.jl:506 [inlined]
 [4] getindex(::CategoricalArray{Int64,1,UInt32,Int64,CategoricalValue{Int64,UInt32},Union{}}, ::InvertedIndex{Array{Int64,1}}) at /home/milan/.julia/dev/CategoricalArrays/src/array.jl:735
 [5] top-level scope at REPL[11]:1
 [6] run_repl(::REPL.AbstractREPL, ::Any) at /builddir/build/BUILD/julia/build/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:288

[mbauman]

Does CategoricalArrays call to_indices?/to_index?

[nalimilan]

AFAICT we only call checkbounds(A, I...):
https://github.com/JuliaData/CategoricalArrays.jl/blob/876d0db0ac11ac338bbb36ac7a92aede7c4074b8/src/array.jl#L735

And indeed checkbounds([1], Not(1)) gives the same error.

[mbauman]

Right, as I recall, we don't implement the iteration/array-like behaviors on Not alone. So it's not just implementing checkindex/checkbounds — it's also implementing iteration and whatnot.

Is there a reason you haven't used to_indices there? Could you add it?

[nalimilan]

No particular reason. I just used the simplest approach I could find. I guess I could do checkbounds(A, to_indices(A, I)...). But that doesn't look optimal: wouldn't that imply that one cannot use checkbounds without calling to_indices first? Maybe we should add a fallback checkbounds definition that calls to_indices?

[mbauman]

No, I was expecting you'd need to do:

I′ = to_indices(A, I)
checkbounds(A, I′)
# implementation using I′ and not I

The reason it's not implemented is because I don't expect implementations to be able to use Nots directly in their indexing implementations. Here, though, you're "saved" because you're not really doing the iteration over the indices yourself — and your implementation is simply deferring indexing to Array, which calls to_indices for you.

Interestingly, I'm not sure what'll happen if you call to_indices on an InvertedIndexIterator... does that work? I don't recall.

[nalimilan]

Yeah that's a particular case where we can let Array do all the work. But even if I called to_indices at some point, I could have written checkbounds(A, I) like I did here, because that's usually the first thing you want to check before doing more work, and there's nothing indicating not to do that. So I think we can improve things somewhere to prevents bugs like this.

Interestingly, I'm not sure what'll happen if you call to_indices on an InvertedIndexIterator... does that work? I don't recall.

Looks like it does:

julia> typeof(to_indices([1], (Not(2),)))
Tuple{InvertedIndices.InvertedIndexIterator{Int64,Int64,Base.OneTo{Int64}}}

julia> typeof(to_indices([1], to_indices([1], (Not(2),))))
Tuple{InvertedIndices.InvertedIndexIterator{Int64,Int64,Base.OneTo{Int64}}}

[mbauman]

Yeah, let's implement checkbounds and then we can add a nicer error hint if someone tries to iterate a bare InvertedIndex.