-
-
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
more methods working with a non-global RNG #8854
Conversation
MersenneTwister's constructor making an instance with an Uint32 seed was removed in commit 4fb4622. But a subsequent call to srand(r::MersenneTwister, seed) would only accept an Uint32 as a seed, and would always make r.seed an Uint32; this was not consistent.
The srand methods working on a MersenneTwister instance did not catch up with those working on the global RNG. They are now made similar thanks to the make_seed function, which implements the different logics. Also, the manual is more precise on the types of valid seeds.
And similarly for rand!, randbool. Fixes #8360.
RFC: more methods working with a non-global RNG
Thanks @rfourquet. How close are we to the original performance with this PR merged? |
In Perhaps we should at this point give up on trying to sync the Rmath RNGs with Julia, and just focus on reducing/removing dependence on Rmath in |
Cc: @JuliaBackports |
Single Julia 0.3.2:
master:
@vtjnash Is there a way to check that |
edit: scratch that, i see it doesn't affect timing |
My understanding from @simonbyrne was that we might not need R's RNG's for Distributions, only deterministic functions found in libRmath. |
FWIW, the timing on my machine before and after this merged appears to be nearly identical for the scalar case, with this PR maybe being slightly faster (I didn't collect enough data to be statistically significant however) |
I was expecting this PR to improve array versions for non |
Yes, this seems to have broken Distributions: |
Random number generation in Distributions.jl is seriously broken. It seems that in addition to JuliaStats/Distributions.jl#293, Binomial distribution also produces wrong results (on Julia 0.4 latest master): julia> using Distributions
julia> p = Binomial(1, 0.5)
Binomial(size=1, prob=0.5)
julia> x = rand(p, 10^6);
julia> all(x .== 0) # if it works well, half of x should be 1, now is produces all zeros!
true |
This should not be backported to 0.3 until we make sure that it works with Distributions.jl. |
@lindahua We're closing the backport window on 0.3.3 soon. Unless you say otherwise, I'm assuming this is not a candidate for backporting at this time. |
Taking off the backport list, since a lot of work has happened on RNGs and much of it is API changes and still stabilizing. |
This is #8380 rebased and on a branch in the main repo.
However I simplified it a bit: the
rand()
methods working with the global RNG were constrained by the types with whichrand(::MersenneTwister, ...)
actually worked. But I think this did not add much value (the "no method" error is simply forwarded), and could easily fall out of sync, so now there is a fall-backrand(T::Type) = rand(GLOBAL_RNG, T)
.I need to review this once more, hence the "RFC".