-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 indexing into Tuple by another Tuple #33306
Conversation
I think the problem is that many people would expect this to be extremely fast, and it's not. julia> t = tuple((10 .* (1:5))...)
(10, 20, 30, 40, 50)
julia> r = (2,4)
(2, 4)
julia> using BenchmarkTools
julia> slowgetindex(t::Tuple, r::Tuple) = ([t[ri] for ri in r]...,)
slowgetindex (generic function with 1 method)
julia> fastgetindex(t::Tuple, r::Tuple) = map(i->t[i], r)
fastgetindex (generic function with 1 method)
julia> @btime slowgetindex($t, $r)
206.451 ns (2 allocations: 128 bytes)
(20, 40)
julia> @btime fastgetindex($t, $r)
0.016 ns (0 allocations: 0 bytes)
(20, 40) The latter is so short you can't take it seriously, it's just a sign that the compiler elided the entire computation. A concern is that we sometimes interpret a tuple as a multidimensional shape, and that's close to interpreting them as a multidimensional index. We have |
Base.getindex(t::Tuple, r::Tuple) = map(i->t[i], r) Any downsides to this faster version? Is the multidimensional indexing with tuples proposal planning to enable the following: julia> t = tuple((tuple(3n-2:3n...) for n in 1:3)...)
((1, 2, 3), (4, 5, 6), (7, 8, 9))
julia> r = (2,1);
julia> t[r]
4 If tuples were to replace
To clarify, is foreseen issue with multidimensional indexing the following?
|
The benchmarking can be made more useful by adding a little setup. julia> @btime fastgetindex($t, s) setup=(s = tuple(rand(1:5, 2)...))
20.056 ns (1 allocation: 32 bytes) So it does seem to be consistently faster, and as fast as TupleTools.jl's recursive julia> @btime TupleTools.getindices($t, s) setup=(s = tuple(rand(1:5, 2)...))
19.470 ns (1 allocation: 32 bytes) I don't quite understand how the concern is relevant here. This PR allows indexing tuples via tuples. So where do we interpret the "value tuple" as an array, in which case it would be indeed ambiguous whether EDIT: If you make |
Yep, you got it. There's also a second issue, that "vector indexing" should perhaps be replaced by broadcasting (#30845) and so in a sense this is taking us in a direction that we probably don't want to be moving (in the long run). While I hate to hold up a good PR for hypotheticals, the flip side is that the implementation of this functionality is so simple that it would be a shame to have it be the source of trouble for more fundamental changes. To clarify, I'm not vehemently opposed to this---it does have a certain consistency. But I am not enthusiastic either. |
Good points. I'm happy to use |
Seems like this behavior would be an added convenience.
Could also allow indexing into AbstractArray by Tuple. Indexing into Tuple by AbstractArray is already supported.
Wanted to get some feedback on whether there are any downsides to this feature before working on whatever other changes need to be made for this to be accepted.
To try locally without incorporating this change: