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

Indexing with an empty vector "regression" #12271

Closed
jiahao opened this issue Jul 22, 2015 · 13 comments · Fixed by #12273
Closed

Indexing with an empty vector "regression" #12271

jiahao opened this issue Jul 22, 2015 · 13 comments · Fixed by #12273

Comments

@jiahao
Copy link
Member

jiahao commented Jul 22, 2015

julia> [1,2][[]] #v0.4
ERROR: MethodError: `_checkbounds` has no method matching _checkbounds(::Int64, ::Array{Any,1})
Closest candidates are:
  _checkbounds(::Any, ::Integer)
  _checkbounds(::Any, ::Real)
  _checkbounds(::Any, ::Range{Int64})
  ...
 in getindex at abstractarray.jl:464

julia> [1,2][[]] #v0.3
0-element Array{None,1}

Prodding a bit more shows that this issue is due to [] now being a synonym for Any[] rather than None[]:

julia> [1,2][Int[]] #both 0.3 and 0.4 ok
0-element Array{Int64,1}:

julia> [1,2][Any[]] #both 0.3 and 0.4 error
ERROR: `checkbounds` has no method matching checkbounds(::Int64, ::Array{Any,1})
 in checkbounds at abstractarray.jl:94
 in getindex at multidimensional.jl:49

@andreasnoack suggests one fix is to relax the typevar restriction in the definition of Base._checkbounds. Doing so leads to

julia> [1,2,3][Any[1]]
ERROR: invalid index: Any[1]
 in error at ./error.jl:21
 in getindex at abstractarray.jl:464

I'm not sure if this is fixable, or worth fixing even. Thoughts?

@yuyichao
Copy link
Contributor

mbauman added a commit that referenced this issue 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.
@hayd
Copy link
Member

hayd commented Jul 23, 2015

Similar weird (?) behaviour:

julia> [1, 2][]
1

julia> [1 2; 3 4][]
1

mbauman added a commit that referenced this issue Jul 26, 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.
@hayd
Copy link
Member

hayd commented Jul 27, 2015

@malmaud Should the behaviour I mention above be a separate issue or is it expected?

@mbauman
Copy link
Member

mbauman commented Jul 27, 2015

@hayd, that behavior is intentional. [] is synonymous with [1]. Its intended use-case is for zero-dimensional arrays and Ref de-referencing, but it works with all arrays (similar to how linear indexing works with multidimensional arrays).

@hayd
Copy link
Member

hayd commented Jul 27, 2015

Thanks!

@StefanKarpinski
Copy link
Member

I'm generally not super jazzed about how many different kinds of indexing we allow into multidimensional arrays. I would prefer it only linear and N-dimensional indexing were allowed. In fact, now that we have generated functions and eachindex, which allow doing N-dimensional indexing in a fast generic way, there's a pretty strong case to be made for dropping linear indexing entirely. But that is a matter to be settled in the Arraymaggeddon.

@mbauman
Copy link
Member

mbauman commented Jul 27, 2015

It can be really powerful, but it's also an easy source of mistakes. I'd definitely be interested in trying it out. I think it could even be deprecated nicely, which makes experimenting with it much easier.

@StefanKarpinski
Copy link
Member

Note that you could still express linear indexing using reshape if you really need it, but not allowing it by default would prevent mistaken usage as well as encourage writing code in a more generic style that will work for non-contiguous arrays. In fact, one could then imagine eliminating the contiguous vs non-contiguous distinction, which would make the array view business much simpler. Are non-contiguous strided arrays closed under slicing with scalars and ranges?

@timholy
Copy link
Member

timholy commented Jul 28, 2015

Now that we have nice iterators I generally dislike linear indexing, but I have the impression that there are still cases where it may be more performant. We still have #9080 to deal with, for example.

@timholy
Copy link
Member

timholy commented Jul 28, 2015

Are non-contiguous strided arrays closed under slicing with scalars and ranges?

If you don't change dimensionality (i.e., use N-dimensional indexing for an N-dimensional array), everything is closed. Once you change dimensionality, all bets are off.

@StefanKarpinski
Copy link
Member

I think there's a lot to be said for only having one strided array type and having that be what is returned by all scalar/range slices, with no copying. We would probably need GC support for interior pointers, but I think we'll need that for strings too, so we may as well do it. For performance, we can always dynamically check whether linear indexing is applicable and do linear indexing if it is. I think the occasional complexity that would add to a few functions would be well worth it for the massive simplification we would get from collapsing arrays, strided arrays and array views into a single type.

@timholy
Copy link
Member

timholy commented Jul 28, 2015

SubArrays pretty much do all that already, except the linear indexing check is done at construction time.

@ScottPJones
Copy link
Contributor

What were you thinking of, for GC support for interior pointers?
Could it simply be only keep pointers to the base of GCed memory, paired with an offset to the actual position?
That's how we had to handle references to shared memory (base pointer + offset), where each process might have the shared memory mapped at a different address.

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 a pull request may close this issue.

7 participants