Replace distribution::Sample with Distribution + polymorphism over Rng #256

dhardy · 2018-02-10T16:31:46Z

If #244 was controversial, this is even more so. There are many commits because there are many changes here, and these are bundled together to make Distribution::sample run-time polymorphic over Rng from the start without affecting Rand.

Replace Sample and IndependentSample with Distribution
Add a Uniform distribution and move all Rand implementations to it
Switch Rng::gen() to use Uniform and deprecate Rand and rand_derive
Merge SampleRng trait code (required to avoid having to type (&mut rng).gen() in all implementations)
Change sample<R: Rng>(..) -> T to sample<R: Rng + ?Sized>(..) -> T

This PR is closely related to dhardy#83. We could potentially keep Rand and fix rand_derive to use Rand::rand(rng) not rng.gen(), or we could even keep Rand over Uniform but make change the method: rand<R: Rng + ?Sized>(..) -> T (breaking existing implementations, but it's still a smaller change for users than deprecating Rand like in this PR). I think I prefer approach since Uniform is a real distribution (uniformity, and maybe someday supporting currying of distributions) and avoids redundant feature overlap.

Comments please on whether you want rand to go in this direction!

dhardy · 2018-02-10T16:45:48Z

To be clear, there are alternatives to this PR:

Not make Rng::gen() polymorphic over Rng (i.e. rng.gen() will not work with rng: &mut Rng, although (&mut rng).gen() appears to work — I don't understand why the latter works)
Keep Rand but change its method to fn rand<R: Rng + ?Sized>(..). This breaks every implementation of Rand so users have to do a similar amount of fixing as with this PR.
Add Uniform and keep Rand. It's not difficult (Rand is only deprecated in this PR), but leaves two different solutions to the same problem.

Also to be clear, Uniform has exactly the same functionality as Rand with slightly different syntax, e.g. see the Option<T> implementation:

impl<T> Distribution<Option<T>> for Uniform where Uniform: Distribution<T> {
    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Option<T> {
        // UFCS is needed here: https://github.com/rust-lang/rust/issues/24066
        if rng.gen::<bool>() {
            Some(rng.gen())
        } else {
            None
        }
    }
}

KodrAus · 2018-02-12T01:06:02Z

@dhardy Just a ping to let you know I'll dig through this, but it might take a few days :)

dhardy · 2018-02-12T11:00:03Z

@sbarral commented on Reddit that it may be worth keeping a distribution trait allowing mutable state since some genuine distribution sampling algorithms use it (e.g. Box-Muller generates two Gaussian samples simultaneously). Since this usage is the exception we agree that the primary distribution trait should not allow mutable state. I'm not sure what the best approach is here but see no reason why we can't leave this to later (i.e. another PR adding DistributionMut or StatefulDistribution or something) — none of the distributions implemented here use mutable state.

KodrAus · 2018-02-15T06:10:15Z

You could also treat DistributionMut like a special case of Distribution:

pub trait DistributionMut<T> {
    fn sample<R: Rng + ?Sized>(&mut self, rng: &mut R) -> T;
}

impl<T, D: Distribution<T>> DistributionMut<T> for D {
    fn sample_mut<R: Rng + ?Sized>(&mut self, rng: &mut R) -> T {
        self.sample(rng)
    }
}

Sorry I've been out of the loop on rand for a while, but this looks like an improvement to me 👍

Something I'm not so sure on are the virtues of the SampleRng extension trait over provided methods on Rng. Is keeping those methods out of scope for backends worth the cognitive load on users? Are there other benefits to keeping Rng itself constrained to that single method? If there's been conversation on this previously I'd be happy to dig through it.

dhardy · 2018-02-15T08:25:42Z

Thanks @KodrAus. There's some discussion in #244 (this PR just merges that one). As noted in the first post, it seems rng.gen() doesn't work if rng: &mut Rng (i.e. using dynamic dispatch) without adding the extension trait, though apparently (&mut rng).gen() does work — why? We could use that instead I guess; presumably it optimises out when using an rng of known type.

Regarding mutable (state-full) distributions, my conclusion from the discussion on reddit is that they won't be needed in the vast majority of cases, and since there's no reason the trait can't be defined externally, I'd rather leave it out of rand.

KodrAus · 2018-02-15T23:38:50Z

Ahh I see, the Rng trait is object safe, but the extensions aren't. serde uses separate object-safe traits that rely on being able to forward object-safe variants of its methods to the non-object-safe originals. If we were going to follow something similar here then Rng would not be object-safe, and there'd be a separate trait like ObjectSafeRng you could use. That doesn't necessarily seem better than the Rng/SampleRng split now, it just slices the traits differently. So what we're doing here seems like a good approach to me.

vks · 2018-02-16T09:44:05Z

Regarding mutable (state-full) distributions, my conclusion from the discussion on reddit is that they won't be needed in the vast majority of cases, and since there's no reason the trait can't be defined externally, I'd rather leave it out of rand.

Also, if in the future there is some need for mutable distributions, this can still be implemented in rand without a breaking change.

@GrahamDennis

This is heavily inspired by #27 by @GrahamDennis but simpler trait and maintains backwards compatibility with deprecations.

Replaces rng.gen() to seed new RNGs with from_rng(&mut rng)

Move default sampling to Default distribution Make Open01 and Closed01 distributions instead of wrappers

This breaks rand_derive because gen() no longer supports derived types.

dhardy · 2018-02-19T16:10:24Z

Rebased after merge of #265.

vks · 2018-02-19T16:40:34Z

Shouldn't random be deprecated as well?

dhardy · 2018-02-19T16:45:15Z

Why? It doesn't need Rand.

vks · 2018-02-19T19:15:54Z

Yes, but it has the same problems that Rand has. It doesn't communicate the distribution.

dhardy · 2018-02-19T19:22:53Z

That is true. Why don't you open a new PR removing it so that it can be discussed there? It's independent of this PR.

random() is no different from rng.gen() in that respect though.

vks · 2018-02-19T19:50:25Z

Yeah, I'm not so happy with rng.gen() either. It is similar to fill though, except that it works for floats and char, where it is maybe better if the user has to specify the distribution. That it works with tuples is fine. I think uniform or sample_uniform would be a better name. The same applies to random.

Why don't you open a new PR removing it so that it can be discussed there?

Will do.

It's independent of this PR.

The deprecations are related, but it works as a follow-up I guess.

dhardy · 2018-02-19T19:55:53Z

I get what you mean about gen not being ideal, but sample_uniform (or rng.sample(Uniform)) is a bit long really. I don't think gen is too bad.

KodrAus · 2018-02-19T21:55:10Z

I like the idea of random being a low-barrier entry point for rand. Does it really need to communicate the distribution in the method name as well as the signature?

pitdicker

I know you already merged this PR.

It looks really good, and I only have two questions.

pitdicker · 2018-02-22T13:48:58Z

src/distributions/normal.rs

        let mut rng = ::test::rng(210);
        for _ in 0..1000 {
            norm.sample(&mut rng);
-            norm.ind_sample(&mut rng);
+            norm.sample(&mut rng);


Is this still a useful part of the test (here and all the similar tests that used ind_sample)?

The duplication is not useful. I noticed this when merging these changes into dhardy/master but didn't fix yet.

As for the tests themselves — good question. I just opened #267.

pitdicker · 2018-02-22T13:51:57Z

src/distributions/float.rs

+
+macro_rules! float_impls {
+    ($mod_name:ident, $ty:ty, $mantissa_bits:expr, $method_name:ident) => {
+        mod $mod_name {


Why create modules here?

I simply moved this code from rand_impls.rs. But I think it does have a purpose — SCALE is a local constant.

Good reason. Then I feel free to remove them 😄

Replace distribution::Sample with Distribution + polymorphism over Rng

dhardy added the E-question Participation: opinions wanted label Feb 10, 2018

dhardy requested a review from KodrAus February 10, 2018 16:46

dhardy mentioned this pull request Feb 15, 2018

Distributions without direct dynamic dispatch #261

Closed

dhardy added 14 commits February 19, 2018 15:40

Replace Sample and IndependentSample with Distribution

8a19d63

This is heavily inspired by #27 by @GrahamDennis but simpler trait and maintains backwards compatibility with deprecations.

Add Rng::sample function

ceefe5c

Remove the RandSample wrapper type

147aa86

Remove impl of Rand SeedableRng impls

e98e62c

Replaces rng.gen() to seed new RNGs with from_rng(&mut rng)

Add the Uniform distribution

7fdf707

Move float sampling from Rand to distributions

d48b40e

Move default sampling to Default distribution Make Open01 and Closed01 distributions instead of wrappers

Make Exp1 a distribution; also expose in distributions module

cfa41f9

Make StandardNormal a distribution; also expose in distributions module

865e4ae

Move integer sampling from Rand to the Uniform distribution

49a4358

Move char and bool sampling from Rand to Uniform

0f8c352

Move remaining Rand impls to the Uniform distribution

18ae3fb

Make Rng::gen() and random() use Uniform.

7dc370f

This breaks rand_derive because gen() no longer supports derived types.

Support run-time polymorphism of Rng in Distribution::sample

3f0892a

Add example and test: distribution usage with type-erased Rng

11b2b45

dhardy force-pushed the distribution branch from 88406d0 to 11b2b45 Compare February 19, 2018 16:09

This was referenced Feb 20, 2018

Replace the Sample and IndependentSample traits with Distribution. #27

Closed

Remove Sized constraint for generic Rng #175

Closed

dhardy merged commit 8ce7435 into rust-random:master Feb 20, 2018

dhardy deleted the distribution branch February 20, 2018 11:06

dhardy mentioned this pull request Feb 20, 2018

Add fill and try_fill methods to Rng #247

Merged

pitdicker reviewed Feb 22, 2018

View reviewed changes

dhardy mentioned this pull request Feb 24, 2018

Tracker: planned changes for 0.5 #232

Closed

33 tasks

pitdicker mentioned this pull request Mar 3, 2018

{Independent,}Sample trait for StandardNormal or rename to Ziggurat #97

Closed

dhardy mentioned this pull request Mar 11, 2018

Fate of the Rand trait dhardy/rand#83

Closed

pitdicker pushed a commit that referenced this pull request Apr 4, 2018

Merge pull request #256 from dhardy/distribution

2b6c326

Replace distribution::Sample with Distribution + polymorphism over Rng

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace distribution::Sample with Distribution + polymorphism over Rng #256

Replace distribution::Sample with Distribution + polymorphism over Rng #256

dhardy commented Feb 10, 2018

dhardy commented Feb 10, 2018

KodrAus commented Feb 12, 2018

dhardy commented Feb 12, 2018 •

edited

Loading

KodrAus commented Feb 15, 2018

dhardy commented Feb 15, 2018 •

edited

Loading

KodrAus commented Feb 15, 2018

vks commented Feb 16, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

KodrAus commented Feb 19, 2018

pitdicker left a comment

pitdicker Feb 22, 2018

dhardy Feb 22, 2018

pitdicker Feb 22, 2018

dhardy Feb 22, 2018

pitdicker Feb 22, 2018

Replace distribution::Sample with Distribution + polymorphism over Rng #256

Replace distribution::Sample with Distribution + polymorphism over Rng #256

Conversation

dhardy commented Feb 10, 2018

dhardy commented Feb 10, 2018

KodrAus commented Feb 12, 2018

dhardy commented Feb 12, 2018 • edited Loading

KodrAus commented Feb 15, 2018

dhardy commented Feb 15, 2018 • edited Loading

KodrAus commented Feb 15, 2018

vks commented Feb 16, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

vks commented Feb 19, 2018

dhardy commented Feb 19, 2018

KodrAus commented Feb 19, 2018

pitdicker left a comment

Choose a reason for hiding this comment

pitdicker Feb 22, 2018

Choose a reason for hiding this comment

dhardy Feb 22, 2018

Choose a reason for hiding this comment

pitdicker Feb 22, 2018

Choose a reason for hiding this comment

dhardy Feb 22, 2018

Choose a reason for hiding this comment

pitdicker Feb 22, 2018

Choose a reason for hiding this comment

dhardy commented Feb 12, 2018 •

edited

Loading

dhardy commented Feb 15, 2018 •

edited

Loading