-
-
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
add mutating versions of randperm & randcycle #22723
Conversation
I changed slightly the behavior: EDIT: If needed, I would propose to be able to also specify directly the type in |
base/random.jl
Outdated
@@ -1799,31 +1800,49 @@ shuffle(a::AbstractArray) = shuffle(GLOBAL_RNG, a) | |||
|
|||
Construct a random permutation of length `n`. The optional `rng` argument specifies a random | |||
number generator (see [Random Numbers](@ref)). | |||
To randomly permute a arbitrary vector, see [`shuffle`](@ref) | |||
To randomly permute an arbitrary vector, see [`shuffle`](@ref) | |||
or [`shuffle!`](@ref). | |||
|
|||
# Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Examples
```jldoctest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for the newline, but do we use plural "Examples" even when there is only one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there's a not yet merged pr that is changing all of the existing cases to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superficially (i.e. modulo interface and always-return-Int
bikeshedding) lgtm! :)
Needs a rebase and CI rerun?
8c2c9c4
to
e6872fa
Compare
Ok, rebased (a previous CI was OK, but then used "[ci skip]" in subsequent commits to avoid overloading nanosoldier). What is your take on the "always-return-Int bikeshedding" ? I don't have a too strong opinion about it, and will revert back (together with adding documentation) if this holds back this PR. The integer type determing the array type makes sense in a way, when considering this |
Can we dig through blame a bit to see when / why that behavior was originally added, see if there's any initial discussion to find? Array element type depending on the type of a length parameter is a little unusual, though I could see it being useful here. |
Excellent idea, which shows that this was not made on purpose if I interpret it correctly. This comes from ae44b3a, with following comit message:
Before, |
I lack a strong opinion; the behavior you propose seems reasonable :). |
I will merge in few days if no objection. |
e6872fa
to
16d11fe
Compare
Another API that could possibly make sense here would be giving the element or array type as the first input, but that's usually not a whole lot more concise than using the mutating versions on a newly-allocated array. |
Yes I mentioned this possibility in my second comment above, but it seems to me that needing a type different than |
This can be useful when needing to call repeatedly one of those functions without allocating excessively.