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

RFC: Allow any index type in nonscalar indexing #12567

Closed
wants to merge 2 commits into from

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Aug 11, 2015

I'm posting this to point an arrow towards where I think the array indexing fallbacks will go. I'm seeking a discussion on what deserves to be changed before 0.4 is branched.

Here's what this enables:

immutable SymInt{T} <: AbstractArray{T,2}
    a::Vector{T}
    b::Vector{T}
    c::Vector{T}
end
Base.size(A::SymInt) = (3, length(A.a))
Base.getindex(A::SymInt, f::Union{Symbol, Int}, i::Int) = getfield(A, f)[i]
Base.checkbounds(A::SymInt, I...) = true # punt

julia> A = SymInt([1,2,3,4],[11,12,13,14],[21,22,23,24])
3x4 SymInt{Int64}:
  1   2   3   4
 11  12  13  14
 21  22  23  24

julia> A[[:c,:a,:b],[3,2,1]]
3x3 Array{Int64,2}:
 23  22  21
  3   2   1
 13  12  11

julia> A[:a, :]
1x4 Array{Int64,2}:
 1  2  3  4

julia> A[:, 2]
3-element Array{Int64,1}:
  2
 12
 22

Here's what I think should occur before 0.4 branches:

  • This sits atop Make checkbounds more consistent & easier to extend #11895. If we can't find a good name for in_bounds, we can at least make it easier to extend checkbounds for a custom type, even if it's still undocumented for custom array types to provide custom index-in-bounds checks.
  • I think we should un-deprecate float indexing

Here's why the rest of this might need to wait, but if folks are gung-ho I can work to mitigate some of these concerns:

  • It adds some crazy powerful functionality that isn't tested (but I can add tests like the above)
  • It changes the behavior of to_index, which, while not documented or exported, does get used outside of base.
  • It uses more generated functions to express dispatch on "at least one vararg is <: X".
  • It extends what it means to be an AbstractArray to a more nebulous area.

@timholy
Copy link
Member

timholy commented Aug 11, 2015

Yeah, let's wait on this for 0.4; I worry about extending indexing semantics right before a release, and Arraymagedon sounds like the perfect time to play with this 😄.

@KristofferC
Copy link
Member

Also, let's benchmark the sh** out of stuff that changes the indexing methods because it seems to be easy to get performance regressions by touching that part of the code base.

@mbauman mbauman force-pushed the mb/10525-on-steroids branch from a40f21f to 31002ea Compare September 30, 2015 23:26
@tomasaschan
Copy link
Member

I knew I'd read about this somewhere! :)

I just want to bump this and check the status. Still in the scope of 0.5? From what I can tell, this is blocking Interpolations.jl from fully utilizing the power of the indexing fallbacks for AbstractArrays.

@mbauman
Copy link
Member Author

mbauman commented Jan 12, 2016

This is a fairly drastic change that abuses generated functions in order to express a form of dispatch that would totally break the decidability of subtyping if we were able to express it as a normal method signature ("at least one vararg is <: Union{AbstractArray,Colon}").

I think we should take small steps toward this. #13614 was something I missed when I created this PR… and it already gets us most of the way here. Here's what I think should be the remaining steps for 0.5:

  • Remove float indexing deprecation. (But don't re-instate converting floats to integers for scalar indexing with the builtin array types.)
  • Allow Numbers in the non-scalar indexing fallbacks, blindly passing the numbers to the scalar indexing methods. The builtin array types will throw an error for non-integer numbers.

I believe that will cover the interpolations use-cases, yes?

@tomasaschan
Copy link
Member

I believe that will cover the interpolations use-cases, yes?

Yes, I believe so. It's really only the fallbacks that affect Interpolations.jl at the moment - we make sure we never index into the underlying array with anything other than Ints (never things like 1.0 either).

Leaning on #13614 definitely sounds like a good idea. Unwrapping mutliple indices into mutliple single-point index operations was exactly what I imagined those fallbacks would eventually do when we discussed this before, so I'm all for it :)

@tomasaschan
Copy link
Member

Referencing JuliaMath/Interpolations.jl#24 here, so I get a link with a badge indicating the status of this PR there :)

@mbauman
Copy link
Member Author

mbauman commented Dec 27, 2016

Superseded by #19730. With that patch, any array can customize it's indexing behaviors very easily and extensibly. For example, to support non-scalar indexing with any Number you can simply wrap them in an AbstractArray{T, 0} within to_index to ensure that they have the correct size and shape upon indexing.

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.

4 participants