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

Trailing-1s indexing with 0-dimensional AbstractArrays #20175

Closed
timholy opened this issue Jan 22, 2017 · 7 comments
Closed

Trailing-1s indexing with 0-dimensional AbstractArrays #20175

timholy opened this issue Jan 22, 2017 · 7 comments
Labels
collections Data structures holding multiple items, e.g. sets

Comments

@timholy
Copy link
Member

timholy commented Jan 22, 2017

Given #19958 and #20079, there may be fixes already submitted for this (or possibly, this won't work in the future anyway), so for now I simply report this:

julia> a = reshape([5])
0-dimensional Array{Int64,0}:
5

julia> a[]
5

julia> a[1]
5

julia> immutable MyArray{T,N} <: AbstractArray{T,N}
           parent::Array{T,N}
       end

julia> b = MyArray(a);

julia> Base.getindex{T,N}(A::MyArray{T,N}, inds::Vararg{Int,N}) = A.parent[inds...]

julia> Base.size(A::MyArray) = size(A.parent)

julia> Base.linearindexing(A::MyArray) = Base.LinearFast()

julia> b[]
5

julia> b[1]
ERROR: indexing not defined for MyArray{Int64,0}
Stacktrace:
 [1] getindex(::MyArray{Int64,0}, ::Int64) at ./abstractarray.jl:792

julia> versioninfo()
Julia Version 0.6.0-dev.2260
Commit b44faa3* (2017-01-22 03:01 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, broadwell)

It succeeds if you omit the linearindexing specialization.

@mbauman
Copy link
Member

mbauman commented Jan 22, 2017

I don't think this is a bug. If you declare your type as a LinearFast, then you are promising that you've defined getindex(::MyArray, ::Int), but the only indexing method you've defined is the LinearSlow one.

I think this is how all the documentation is written, and I don't think it's worth adding it's possible to add a special case for this (without running into stack overflow errors). We could probably make that error method a little clearer, though.

@mbauman
Copy link
Member

mbauman commented Jan 22, 2017

What would you think about renaming LinearFast to simply LinearIndexing? And LinearSlow to CartesianIndexing? I think that's part of the confusion. That trait has become more about what indexing methods the arrays natively support than it is about storage layout. It'll take a bit more brainstorming, though, since those new names won't work in one step.

@timholy
Copy link
Member Author

timholy commented Jan 22, 2017

Yes, having them mean "what methods I need to implement" would convey a lot of useful information to package developers. Great idea. If we wanted to do it in one shot, we could reverse word order and use IndexedLinearly and IndexedCartesianly. OK, I admit that last is a little awkward. If you don't like it, it's at least a candidate for a transitional name.

@ararslan ararslan added the collections Data structures holding multiple items, e.g. sets label Jan 23, 2017
@mbauman
Copy link
Member

mbauman commented Jan 23, 2017

Proposal: use the word index like a verb:

LinearIndexing -> IndexMethod (the abstract super)
LinearFast -> IndexLinear
LinearSlow -> IndexCartesian

@andyferris
Copy link
Member

LinearIndexing and CartesianIndexing seems better to me, possibly along with IndexingMethod.

I feel that introspection works a bit like a conversation with the type/object:

"What is your IndexingMethod?"

"LinearIndexing"

"Thanks. In that case, give me element 1"

...

@timholy
Copy link
Member Author

timholy commented Jan 23, 2017

@mbauman, you're right about the fact that this isn't a bug. Turns out to have arisen in a package that formerly worked by overloading Base._getindex, and the order of checking for errors vs dispatch changed in 0.6. Since that's an internal detail, I'll close this.

@andyferris, we didn't explain that LinearIndexing already means something (it's an abstract type), so we need to choose a different name so we can deprecate the meaning of the old one.

@timholy timholy closed this as completed Jan 23, 2017
@andyferris
Copy link
Member

Right you are - that's a pity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets
Projects
None yet
Development

No branches or pull requests

4 participants