Skip to content
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

Random: define Sampler from types, not objects #28010

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Conversation

rfourquet
Copy link
Member

Currently, one defines a sampler on a type with Sampler(rng::AbstractRNG, x, r) = .... This changes this to Sampler(RNG::Type{<:AbstractRNG}, x, r).

The reasoning is that an instance of an RNG shouldn't be used in this function, i.e. only the type is enough to create a Sampler object, which can then be used by different rng instances of the same type.

For convenience, a fall-back Sampler(rng::AbstractRNG, x, r) = Sampler(typeof(rng), x, r) is defined. This is similar to what eltype does.

@rfourquet rfourquet added the randomness Random number generation and the Random stdlib label Jul 9, 2018
@StefanKarpinski
Copy link
Member

I appreciate improvements, but we've really got to start enforcing feature freeze even for stdlibs very soon. How many people/packages is this likely to disrupt? How many more changes like this do you anticipate needing to make?

@rfourquet
Copy link
Member Author

Yes, I'm embarassed to propose again one more late change. It's the feedback I got recently on the Random.gentype proposal that prompted me to come up with this. I don't anticipe any other change for 0.7.

Technically, it's quite a simple patch which doesn't appear risky, and I believe not a single person or package will be disrupted with this, as the Sampler stuff was implemented in 0.7 and never documented till now (although it's possible to find information about it in a couple of places on github).

So I think it's a good idea to get this in now for beta2; I will merge later today if no objection.

@StefanKarpinski
Copy link
Member

Ok, that sounds good. I'm relieved that you don't have a pile more of these 😸

@ararslan ararslan mentioned this pull request Jul 10, 2018
7 tasks
@rfourquet rfourquet merged commit 5d1a3fc into master Jul 10, 2018
@vtjnash vtjnash deleted the rf/sampler-rngtype branch July 10, 2018 21:08
rfourquet added a commit to JuliaRandom/RandomExtensions.jl that referenced this pull request Jul 24, 2018
rfourquet added a commit to JuliaRandom/RandomExtensions.jl that referenced this pull request Jul 24, 2018
rfourquet added a commit to JuliaRandom/RandomExtensions.jl that referenced this pull request Jul 24, 2018
rfourquet added a commit to JuliaRandom/RandomExtensions.jl that referenced this pull request Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants