-
-
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
Argument order for rand! #8246
Comments
This has been discussed somewhere before and I think @johnmyleswhite gave the answer. Personally, I don't like the rule that the mutated argument should be the first one. It could be awkward for some functions and for all the matrix multiplication and division functions I'd prefer that we followed the BLAS conventions strictly and that would mean that the mutating argument was last (as well as adding scalar α and β arguments). For some linear algebra functions you'd also want to give a workspace array as argument to avoid reallocation and such an argument would make more sense as the last argument. |
I'm happy with any rule personally. Whether the mutated arguments are first or last doesn't matter much to me, although the argument against putting mutated arguments last is that it breaks functions with varargs. |
I don't have a preference on the rule, just that there should be one, or that we should get #249. But I also think at this point we don't want to break all code that uses mutating functions by moving the output last. As far as matrix functions, maybe α, β, and workspace should be keyword arguments? Although right now that incurs kwsorter overhead. |
Perhaps there could be a convention where the names of mutating arguments end in an exclamation point? rand!{T}(r::Range{T},A::AbstractArray{T,N}) would become rand!{T}(r::Range{T},A!::AbstractArray{T,N}) in docs and when calling |
I don't think there are any established conventions, here or elsewhere. While BLAS puts it last, more often than not libc puts them first ( I would also conceptually distinguish |
Better link for the reason for separating output from input: #7513 (comment) |
@timholy I can't see the performance advantage for triangular solve on my machine julia> minimum([@elapsed (for i = 1:100;MyTest.mysolve1(A, b, x);end) for j = 1:10])
0.306543869
julia> minimum([@elapsed (for i = 1:100;copy!(x,b);MyTest.mysolve2(A, x);end) for j = 1:10])
0.305641564 I'm not that surprised about the result. Efficiency seems to have been a pretty high priority in the development of BLAS. The code is module MyTest
function mysolve1{T,S}(A::Triangular{T,S,:L,false}, b, x = b)
n = size(A, 1)
Ad = A.data
@inbounds begin
for j = 1:n
xj = b[j]
for i = 1:j-1
xj -= Ad[j,i]*x[i]
end
x[j] = xj/A[j,j]
end
end
x
end
function mysolve2{T,S}(A::Triangular{T,S,:L,false}, b)
n = size(A, 1)
Ad = A.data
@inbounds begin
for j = 1:n
bj = b[j]
for i = 1:j-1
bj -= Ad[j,i]*b[i]
end
b[j] = bj/A[j,j]
end
end
b
end
end |
@simonster I really think it is a mistake that we haven't followed the BLAS convention completely and I also hope that we can still break code to make things right. Actually, I began the work to change the convention some time ago, but the development stalled in discussion. |
Sure, for this kind of algorithm it won't matter. Compare the running time to that of But that doesn't mean that it's a good model to apply to other algorithms that may be cache-limited on the output. |
All
Note that if the So it makes sense that an output array |
To insist on the "output" characteristic of |
|
Actually that's wrong: the argument order for |
It's also arguable that any variant of |
@simonster The same argument was tried on IO functions, but I think it was decided that even though most of them modify a IO object, only those utilizing pre-allocated memory, should have the |
And it would be less useful: the bang It would be nice if |
Here's my personal list of advantages for either order. Please add to it. Since there is no convention among C libraries, I'm not including "BLAS does it that way" because one can equally well say "libc does it the other way". Advantages of output first:
Advantages of input first:
To me, the varargs/ |
Completely agree with @timholy: the varargs case strongly argues for putting all output arguments at the front. |
Ah, and reading up I see you made that same point at the top. Could have saved myself some time... |
Given that we are on this path, I guess we just need to go ahead and change the calling sequences for |
+1 |
@rfourquet Since you have considerably improved the RNG codebase and are the one most familiar with it, would it be possible for you to create a PR for this change? |
Yes I will do that. But now that objects can be
This would imply having the |
I like the suggestion. Would love to hear what others have to say. |
I also like it. How would this fit into Distributions.jl? |
I don't like overloading |
This could be interesting, but I'm slightly skeptical. What does |
Yes |
The current syntax to fill an array |
Given the proposed rule that outputs must always come first, I'd prefer |
While I like the consistency, Putting the state argument second doesn't appeal aesthetically. I would love to have |
Well, the call does actually mutate both of the two first arguments :) |
I feel like having the RNG anywhere but first or as a keyword arg looks really odd. |
If we look at this from a dot oriented language, it would clearly be written as |
|
change argument order for rand! (fix #8246)
The API to fill randomly an array A is changed from rand!([rng], [::Range], A) to rand!([rng], A, [::AbstractArray]). Enabling [::AbstractArray] instead of only [::Range] depended on choosing first the argument order.
The API to fill randomly an array A is changed from rand!([rng], [::Range], A) to rand!([rng], A, [::AbstractArray]). Enabling [::AbstractArray] instead of only [::Range] depended on choosing first the argument order.
Is there a reason that the output array comes last instead of first as is the case for most other mutating functions? Right now we could change the argument order and add a deprecation, but we should decide whether we want to change this before #6003.
The text was updated successfully, but these errors were encountered: