Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multidimensional array indexing with any eltype #12273

Merged
merged 2 commits into from
Jul 27, 2015
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jul 23, 2015

This enables the use of the multidimensional abstract array indexing fallbacks for all element types. In particular, this fixes #12271: A[[]]. It also allows indexing with A[Any[1,2,3]]. I think this is a good direction in general, since we simply index through the array and pass along the elements to a scalar indexing method. If the AbstractArray subtype doesn't support indexing with those elements, then it'll throw an error there. But I think this starts allowing fancier capabilities in packages, like indexing with an array of Dual numbers.

@@ -316,7 +316,7 @@ to_index(c::Colon) = c
to_index(I::AbstractArray{Bool}) = find(I)
to_index(A::AbstractArray{Int}) = A
to_index{T<:Integer}(A::AbstractArray{T}) = [to_index(x) for x in A]
to_index(i) = error("invalid index: $i")
to_index(i) = i
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose doing it this way makes for some less intelligible error messages. We could be a little less lenient and keep the error, and add to_index(i::AbstractArray) = i.

@mbauman
Copy link
Member Author

mbauman commented Jul 23, 2015

Ref #10525 (comment) and cc @timholy, @tlycken

@tomasaschan
Copy link
Member

Nice! I had some test breakage in Interpolations.jl only yesterday because I tried to do something like itp[1:.1:15], which didn't work out of the box - I suppose this might fix that for me. Will have to take it for a spin... :)

@tomasaschan
Copy link
Member

Nope, same error still:

julia> Pkg.checkout("Interpolations") # #10525 isn't utilized in the latest tagged version
julia> using Interpolations
julia> A = sin(1:20)
julia> itp = interpolate(A, BSpline(Quadratic(Flat)), OnGrid)
julia> itp[3:.4:15]
ERROR: MethodError: `*` has no method matching *(::Array{Float64,1}, ::Array{Float64,1})
Closest candidates are:
  *(::Any, ::Any, ::Any, ::Any...)
  *{T<:Union{Float64,Complex{Float32},Float32,Complex{Float64}},S}(::Union{DenseArray{T<:Union{Float64,Complex{Float32},Float32,Complex{Float64}},2},SubArray{T<:Union{Float64,Complex{Float32},Float32,Complex{Float64}},2,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD}}, ::Union{SubArray{S,1,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD},DenseArray{S,1}})
  *{TA,TB}(::Base.LinAlg.AbstractTriangular{TA,S<:AbstractArray{T,2}}, ::Union{DenseArray{TB,2},SubArray{TB,2,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD},SubArray{TB,1,A<:DenseArray{T,N},I<:Tuple{Vararg{Union{Colon,Int64,Range{Int64}}}},LD},DenseArray{TB,1}})
  ...
 in getindex at /Users/tlycken/.julia/v0.4/Interpolations/src/b-splines/indexing.jl:46

I think the problem is that since we allow indexing with Any, our indexing for ranges don't fall back to the AbstractArray defaults from #10525, and thus tries to treat the index range as a scalar. We don't seem to have done anything fishy, though:

julia> methods(getindex, Tuple{Interpolations.AbstractInterpolation})
3-element Array{Any,1}:
 getindex{T,N}(itp::Interpolations.BSplineInterpolation{T,N,...}, xs...) at /Users/tlycken/.julia/v0.4/Interpolations/src/b-splines/indexing.jl:27
 getindex{T,N,...}(exp::Interpolations.AbstractExtrapolation{T,N,...}, xs...) at /Users/tlycken/.julia/v0.4/Interpolations/src/extrapolation/indexing.jl:9
 getindex(A::AbstractArray{T,N}, I...) at abstractarray.jl:463

What would be the best way to fix this? Do we need to add a method in Interpolations.jl, or should something rather be changed in base?

@mbauman
Copy link
Member Author

mbauman commented Jul 23, 2015

I think the problem is that since we allow indexing with Any, our indexing for ranges don't fall back to the AbstractArray defaults

Ah, shucks. Of course. This would require dispatch on complements of Union. I suppose Interpolations could opt in to the fallbacks for Union{Int,AbstractArray,Colon} by explicitly dispatching to the internal undocumented methods... But that's intentionally not a stable API.

The other thing I realized is that you may run into trouble with element types since interpolating doesn't always return the eltype, but the array indexing methods preallocate an output array using eltype.

@tomasaschan
Copy link
Member

I suppose Interpolations could opt in to the fallbacks for Union{Int,AbstractArray,Colon} by explicitly dispatching to the internal undocumented methods... But that's intentionally not a stable API.

This doesn't seem like a viable long-term solution, so I'll just hold off on it for now. But it would be awesome if base could learn to handle these things :)

The other thing I realized is that you may run into trouble with element types since interpolating doesn't always return the eltype, but the array indexing methods preallocate an output array using eltype.

We've gone quite some length to make sure that the T in the AbstractArray{T} type each interpolation type inherits actually corresponds to the type returned by indexing into the interpolation object if you're indexing with "reasonable" number types. We've also provided methods for interpolate where you can provide the T yourself, if you know you're going to index with some esoteric number type and want to make sure things Just Work(TM), so if users run into problems here I think we've catered for their needs too, even if it will require a small effort on their part.

@timholy
Copy link
Member

timholy commented Jul 23, 2015

I think the problem is that since we allow indexing with Any, our indexing for ranges don't fall back to the AbstractArray defaults

In Interpolations, if we want to make use of the fallbacks in #10525 we should probably restrict the index type to Number.

I approve of this PR.

@mbauman mbauman changed the title RFC: Allow multidimensional array indexing with any eltype WIP: Allow multidimensional array indexing with any eltype Jul 23, 2015
@mbauman
Copy link
Member Author

mbauman commented Jul 23, 2015

I want to simplify things here a bit further before this gets merged.

This enables the use of the multidimensional abstract array indexing fallbacks for all element types.  In particular, this fixes #12271: `A[[]]`.  It also allows indexing with `A[Any[1,2,3]]`.  I think this is a good direction in general, since we simply index through the array and pass along the elements to a scalar indexing method.  If the `AbstractArray` subtype doesn't support indexing with those elements, then it'll throw an error there.  But I think this starts allowing fancier capabilities in packages, like indexing with an array of Dual numbers.
I think the behavior here is more flexible and gives better error messages at the same time:

```jl
julia> A[[]]
0-element Array{Int64,1}

julia> A[im]
ERROR: indexing Array{Int64,3} with types Tuple{Complex{Bool}} is not supported
 in error at ./error.jl:21
 in getindex at abstractarray.jl:463

julia> A[[im]]
ERROR: ArgumentError: unable to check bounds for indices of type Complex{Bool}
 in getindex at abstractarray.jl:463

julia> A[Vector[[1,2],[3,4]]]
ERROR: ArgumentError: invalid index: Array{T,1}[[1,2],[3,4]]
 in getindex at abstractarray.jl:463
```
@mbauman mbauman changed the title WIP: Allow multidimensional array indexing with any eltype Allow multidimensional array indexing with any eltype Jul 26, 2015
@mbauman
Copy link
Member Author

mbauman commented Jul 26, 2015

Ok, this is much better now:

julia> A[[]]
0-element Array{Int64,1}

julia> A[im]
ERROR: indexing Array{Int64,3} with types Tuple{Complex{Bool}} is not supported
 in error at ./error.jl:21
 in getindex at abstractarray.jl:463

julia> A[[im]]
ERROR: ArgumentError: unable to check bounds for indices of type Complex{Bool}
 in getindex at abstractarray.jl:463

julia> A[Vector[[1,2],[3,4]]]
ERROR: ArgumentError: invalid index: Array{T,1}[[1,2],[3,4]]
 in getindex at abstractarray.jl:463

We're still not quite at the point where we can support any element types for non-scalar indexing. The goals of scalar and non-scalar indexing fallbacks are slightly different.

Scalar indexing:

  1. Convert given indices to Int with to_index
  2. checkbounds
  3. Use sub2ind or ind2sub and call getindex with all Ints.

Non-scalar indexing:

  1. Call checkbounds first so ranges and logical vectors are optimized
  2. Iteratively call scalar getindex with the elements of the index arrays

For non-scalar indexing, we don't really care what the element types of the index arrays are... except for that hoisted checkbounds call. I'm still not sure how to best deal with that, but for now custom arrays could potentially override checkbounds for checking custom indexing behaviors.

Anyhow, that's an aside and a project for another time... and really just notes for my future self.

checkbounds(A, I...)
unsafe_getindex(A, sub2ind(size(A), to_indexes(I...)...))
J = to_indexes(I...)
checkbounds(A, J...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the check before calling to_indexes is better-optimized for logical indexing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, but this is scalar indexing so I<:Tuple{Vararg{Real}}. It's something that I think we can get rid of once float indices are no longer supported, but that will take some thought about what to do with non-Int integers. We'll still want to convert small unsigned integers to Int before doing sub2ind arithmetic... and ideally also allow larger integers, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point---I was still remembering the original thinking behind this code and not noticing what it had become. Great work as always.

@JeffBezanson
Copy link
Member

Ready to merge?

@mbauman
Copy link
Member Author

mbauman commented Jul 27, 2015

Yup!

mbauman added a commit that referenced this pull request Jul 27, 2015
Allow multidimensional array indexing with any eltype
@mbauman mbauman merged commit 3a5ffc7 into master Jul 27, 2015
@mbauman mbauman deleted the mb/anyarrayindex branch July 27, 2015 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexing with an empty vector "regression"
4 participants