add rand(::IntSet) #21960
add rand(::IntSet) #21960
Conversation
|
I'm not sure we're consistent enough about the max element of an IntSet for this to make sense. We've been treating it as an unbounded set. |
|
|
|
I also don't understand what the problem is with the "max element"... There is a well defined number of elements in an |
|
My apologies, I didn't look closely enough. I was somehow thinking |
|
Oh you remark make more sense now if you were thinking |
|
I realized that the non-scalar methods (creating/filling arrays) were missing for |
|
Good to go? |
base/random.jl
Outdated
| * a type: the set of values to pick from is then equivalent to `typemin(S):typemax(S)` for | ||
| integers (this is not applicable to `BigInt`), and to ``[0, 1)`` for floating point numbers; | ||
|
|
||
| `S` defaults to `Float64`. | ||
|
|
||
| ```jldoctest | ||
| julia> srand(0); rand(Int, 2) |
There was a problem hiding this comment.
I'm not sure whether srand(0) is needed here, for the doctests?
There was a problem hiding this comment.
I would think yes, since otherwise the results would be different every time.
There was a problem hiding this comment.
So I saw that it's also possible to use a julia-repl block instead of jldoctest. Could be cleaner...
There was a problem hiding this comment.
Downside of that is that they wont be tested. On the other hand, these functions might be fundamental enough that they wont change. julia-repl is fine IMO.
There was a problem hiding this comment.
Instead of srand in the doctest you can use a meta block: https://github.com/JuliaLang/julia/search?l=Markdown&q=meta&type=&utf8=%E2%9C%93
|
Rebased. I will merge soon if no more comments come. |
| while true | ||
| n = rand(r, rg) | ||
| @inbounds b = s.bits[n] | ||
| b && return n |
There was a problem hiding this comment.
will this actually be uniform?
There was a problem hiding this comment.
I believe so, and this is how other uniform random functionality are implemented (rand on a range, on Dict, etc.). Basically pick uniformly one of the possible positions, and if it's not valid try again.
test/random.jl
Outdated
| a2 = rand(rng..., C, 2, 3) ::Array{T, 2} | ||
| a3 = rand!(rng..., Array{T}(5), C) ::Vector{T} | ||
| a4 = rand!(rng..., Array{T}(2, 3), C) ::Array{T, 2} | ||
| for a in [a0, a1..., a2...] |
There was a problem hiding this comment.
Good catch, looks like unintentional omission.
| 1339893410598768192 | ||
| 1575814717733606317 | ||
|
|
||
| julia> rand(MersenneTwister(0), Dict(1=>2, 3=>4)) |
There was a problem hiding this comment.
If this is not a doctest perhaps the MersenneTwister(0) is unnecessary
There was a problem hiding this comment.
Actually, I think I wanted to illustrate different combinations of arguments and show the use of an explicit RNG (and MersenneTwister is the most likely to be used, but doesn't accept for now no-argument), so I would rather keep it as is.
And change "dict" -> "collection" in the exception, as it is also used from rand(::Set).
|
Rebased. Will merge when CI turns green again. |
The second commit optimizes
rand(::Dict)by using aRangeGeneratorobject, the speed-up seems to be roughly +50%.