RFC: controlling dispatch with varargs of defined length #10691
Conversation
| @@ -471,6 +472,37 @@ static jl_value_t *intersect_tuple(jl_tuple_t *a, jl_tuple_t *b, | |||
| } | |||
| jl_tupleset(tc, ci, ce); | |||
| } | |||
| // Check for a length-constrained vararg | |||
| if (bseq) { | |||
| if (!jl_is_long(bn)) | |||
|
Excellent PR! This would be cool to have, as it basically generalizes Tuple and NTuple into a single thing. The implementation seems pretty good. Unfortunately this conflicts to an extreme degree with #10380. There I removed the |
|
Very cool. I've not looked in depth yet, but does this mean that we'd be able to define things like: getindex{M, N}(A::AbstractArray, I::Integer...M, index::CartesianIndex{N})which would allow us to generalize cartesian indexing over "inner" regions? |
|
No; I meant to imply that the generalization to allowing |
|
Least important part, but I'm not entirely sold on the syntax. |
|
I recall a suggestion that we use But we have to be careful about the following case. If I have I should not be able to write |
|
It seems like this should be spelled |
|
Where? Let's just look at the names in question, rather than maligning my naming habits in general. Everything is open to kvetching, but if one does not even kvetch, not much can be done. I think this case is debatable. I would argue that "vararg" has become a single programming term. Googling it gives many hits, even where it is spelled "Vararg". |
Fixes a stack overflow in parsing atsign-`sprintf "%f %d %d %f" 1.0 [3 4]... 5`
|
@JeffBezanson, sorry about the conflict with your awesome tuple work. I'm open to changes here if it simplifies getting both of these together. I pushed a small commit addressing your review comments, and also had to prevent parsing matches if there is a space between For me locally this passes all tests, and if that's also true for our CI then I suppose the question turns to "should we merge this now, or later?" So,
|
|
If we do this, it's probably my branch that needs to change. Ideally we can keep Tuple and Vararg, and move NTuple out of the core. In my redesign, there is only a boolean flag and no room for an extra parameter (was hoping to save a few objects that way, but oh well). Another possible syntax is We should consider how this maps to machine data types. Maybe there are some source files that should trigger a |
I'm fine with that; I just went with
I don't (yet) see why it wouldn't be the same. (Is
Those seem like reasonable choices. I suppose we could even have a file (called |
|
@JeffBezanson, sorry, I didn't mean to malign but to raise awareness so that next time you're naming something you may pause and consider if you're doing that (after raising your fist in the air and saying "dammit, Stefan"). It's entirely possible I'm imagining such a tendency as well. I do agree that "varargs" is a borderline case and one could argue that "vararg" is a word at this point. I'm pretty sure that Google ignores capitalization so I doubt that test really indicates much about which form is more popular. |
|
I'd like to advocate for introducing this functionality without any convenient syntax. If we find ourselves using it a lot, then we can pick a surface syntax that is just a nicer way of writing |
|
I'll look into that; clearly merits investigation first. (I'm on my branch, but I'm almost certain this happens on master, too.) |
|
i don't understand the motivating example for this. can you sketch a more complete example of where this is needed for method dispatch? |
|
@vtjnash, the bigger picture on #10525 (one of my favorite PRs in recent memory) is to centralize all the "hard" stuff in indexing, and allow users to write new AbstractArray types and handle just the simplest indexing operations. As one example, julia allows one (and users find it convenient) to index a 3-dimensional array with 1, 2, 3, or 4 indexes (the 4th has to be 1). One can also index them with Ranges, Vectors, boolean arrays, etc. There are many outstanding bugs/complaints about AbstractArray types that don't implement the full range of indexing operations. As a case in point, until I reworked SubArrays for 0.4, they didn't handle indexing with the "wrong" number of indexes; likewise, ArrayViews (a justifiably well-regarded AbstractArray class) doesn't handle many core indexing methods like The central idea in #10525 is to massage whatever indexes the user provides into a form that makes it easy for the package author. Let's say you have an AbstractArray class for which linear indexing is slow but cartesian indexing is fast (e.g., SubArrays, InterpolationArrays, etc). So indexing a 3d array with 1,2,3, or 4 scalar indexes has to turn into fast variants of More complex contructs are needed for If the user writes a method then she is only intending to handle the scalar case, and rely on the centralized code to handle the loops needed for |
|
It may be time to consider having indexing take a single tuple argument
instead of varargs. After the upcoming change tuples will probably be
efficient enough.
|
|
@timholy thanks. that's a good summary. i was confused initially and thought you were trying to ensure the user could cause no-method exceptions (based on the tests)
the representation of NTuple is currently undefined, but I usually assume it should be the same as the a Tuple of the specified number of arguments
Given the above statement, it seems that one of
This seems like it would be stored as a flat tuple with n+1 Int members in-line in memory
There's also the question of what happens when this gets passed to a function. Most efficient, would probably be to pass this as a single tuple/vector (same as if the user had passed a single literal Tuple). |
|
OK, I figured out why declarations like I think all that's left is to make a final decision about the syntax (or drop it altogether, as suggested by Stefan). |
|
To immediately go against my own plea for ignoring syntax at this point, we could use the kind of notation that's been bandied about in relation to #10380, we could have the syntax |
|
To clarify, this PR equates Otherwise, though, there is a certain appeal to your proposed syntax. It will be interesting to hear how it competes with other possible uses of |
|
It won't always follow Seems to me even the |
|
Yes, as long as one can pack and unpack tuples "for free" then I agree they make a nice interface--- If one goes with tuples, the only downsides I see:
|
|
Should I just strip the syntax and merge a version of this that uses |
|
Dicts are an interesting comparison because by and large everybody thinks indexing a Dict with multiple indexes can only be equivalent to indexing it with a single tuple. Switching arrays to tuple indexing would make the interfaces match; only difference would be arrays are optimized for dense storage. This bears careful thinking. I've always felt there is something weird and annoying about varargs. They have this non-first-class feel. For example consider the slightly odd argument order in |
|
I've always felt that it would be nicer to do |
|
Yes I have the same recollection. |
|
This is going to be tricky to deprecate :-\ |
|
All this resonates with me, too. But it clearly bears very careful consideration. Aside from the challenging deprecation (i.e., focusing on whether we want to do this), one minor annoyance might be for people who create immutable ToeplitzMatrix{T} <: AbstractMatrix{T}
offdiags::Vector{T}
midpoint::Int
end
getindex(A::ToeplitzMatrix, i, j) = A.offdiags[i-j+A.midpoint]The latter would presumably have to become getindex(A::ToeplitzMatrix, I) = A.offdiags[I[1]-I[2]+A.midpoint]which is not quite as pretty on the right hand side, but is something I can imagine everyone getting used to. |
|
Argument destructuring would help a lot since you could then write this: getindex(A::ToeplitzMatrix, (i, j)) = A.offdiags[i-j+A.midpoint] |
|
Excellent point, and there's a feature request open for that already. |
|
+1 to merging this functionality into 0.4 and I also support the proposals / suggestions regarding array indexing etc. |
|
We don't necessarily need both. |
|
If the tuple work will be completed and the window for 0.4 left open a bit longer, then I'd be in favor of exploring the tuple proposal for handling array indexing. If that doesn't end up looking promising, presumably we can merge something like this. |
|
I should also ask: if we do this with tuples, will the declaration need to use triangular dispatch, i.e., getindex{T,N,TT<:NTuple{N,Number}}(A::MyArray{T,N}, indexes::TT) = ...As an "interesting" case, think about the situation in which a user wants to compute the derivative along dimension 2 for an v = A[3.2, dual(1.8,1)] # dual from DualNumbers.jlso the elements of the If we need triangular dispatch but it's not going to happen in 0.4, then we might need to reconsider. |
|
Isn't this just equivalent to |
|
Hmm, I'd temporarily forgotten tuples are covariant, unlike other types in julia. Good. |
|
Closed in preference to #10911. |
This implements the syntax
x...Nto specify a varargs argument of lengthN. The purpose is to be able to control dispatch to functions likeso that this method only gets called when the number of
indexesmatches the dimensionality of the array. This is motivated by #10525.Incidentally, this adds a number of comments to "core" code, particularly
inference.jl.