Make checkbounds more consistent & easier to extend

[mbauman]

• Remove index types from all checkbounds methods to make it easier for custom arrays to extend checkbounds without ambiguities.
• Provide a nicer name for custom array types to extend and override if they want to provide an optimized in_bounds check for the dimension that they are indexing into (which simply returns true or false without throwing errors).

The in_bounds name already existed in base with only one use and the same meaning. This simply uses it more consistently and in a way that makes it easier to extend. It remains un-exported, and un-documented for now.

• Remove index types from checkbounds(A::AbstractArray, I...) to make it easier for custom arrays to extend checkbounds without ambiguities.
• Rename the internal _checkbounds(sz::Integer, I) to checkbounds(::Type{Bool}, sz::Integer, I) in order to provide a nicer name for custom array types to extend and override if they want to provide an optimized bounds check for the dimension that they are indexing into (which simply returns true or false without throwing errors).
• The internal in_bounds is folded into checkbounds(::Type{Bool}, …).

Worked through in collaboration with @davidagold for a NullableArrays use-case, similar to how DataArrays currently works (c.f. https://github.com/JuliaStats/DataArrays.jl/blob/e7172be39310728acc4ded16a988eae603b69787/src/indexing.jl#L103-L113).

[ScottPJones]

Sounds very good to me!

[mbauman]

Alright, rebased. I haven't championed this guy all that much since it does cause some undocumented churn over at DataArrays. But I do think this is important.

Should we document and export in_bounds? It's a little confusing with @inbounds.

[timholy]

I like this.

Should we document and export in_bounds? It's a little confusing with @inbounds.

What about making in_bounds(a, ...) become checkbounds(Bool, a, ...)?

[mbauman]

I'm not sure about that. I think the rest of the arguments are different enough from checkbounds that it'd be confusing. I think we could replace the last in_bounds(dims::Dims, I...) method here with checkbounds(::Type{Bool}, A::AbstractArray, I...), but the others specifically check to see if just one index is between 1 and sz, with the intention that indices will overload them.

It would be nice not to export a new name, for sure.

[mbauman]

Maybe call in_bounds here index_inbounds to express more clearly that this is for dispatch on the indices and not the array being indexed?

[timholy]

That still sounds like I'm checking a specific index.

Since !(Integer <: AbstractArray), I still wonder if checkbounds could be used for this:

# Throw an error if false
checkbounds(A, indexes...)
checkbounds(size(A,d), i)

# Return true if in-bounds, false otherwise
checkbounds(Bool, A, indexes...)
checkbounds(Bool, size(A,d), i)

So you can pass either an AbstractArray or the size of a dimension of that array.

[mbauman]

Yes, that seems like a nice API. Then custom types that want to have special indexing behaviors override checkbounds(::Type{Bool}, ::CustomArray, I…), whereas custom types that want to be used as indices in any array override checkbounds(::Type{Bool}, ::Integer, I::CustomIndex).

The trouble is this allocates a tuple for some reason. Minimal example:

f(I...) = f(Bool, I...)
f(::Type{Bool}, I) = I
@code_llvm f(1)

Edit: Ugh, wait, I think this might be an artifact of testing these functions at global scope directly instead of within a secondary function.

[mbauman]

I like this, but it changes semantics to a form that seems to be slightly slower in my preliminary tests. :-\

Since we want to only require folks to extend one method, it effectively changes things from:

check_dim_1() || error
check_dim_2() || error
check_dim_3() || error

to:

(check_dim_1() && check_dim_2() && check_dim_3()) || error

I haven't dug into this much further, but it'll have to wait until I can be sure that this re-arrangement doesn't incur any penalty.

[timholy]

Yeah, don't want a penalty. I didn't take the time to dig into the details, but that sounds like something that can be avoided---presumably the checkbounds(A, I...) function can work one dimension at a time rather than just calling out to checkbounds(Bool, I...).

[mbauman]

Ok, finally updated once again. I went back to the original code semantics and simplified things considerably. It's now almost entirely just a renaming of checkbounds. I can no longer measure any performance impact.

If this can get this squeezed into 0.4 it'll make life a little nicer for DataArrays and NullableArrays.

[timholy]

Really nice! I approve whenever you're ready to merge.