Add tonnes of impls for Rand. #148

clarfonthey · 2017-05-13T21:51:16Z

I'm not sure how many of these we want, but I just looked through the list and implemented as many as I found appropriate. I'm more than happy to prune the PR of unwanted impls.

nagisa · 2017-05-14T00:33:47Z

src/rand_impls.rs

+            Range { start: end, end: start }
+        } else {
+            Range { start: start, end: end }
+        }


Not exactly sure why you’d cut half of the range here. Range with end < start does make sense in some contexts and the type in question has no real requirements wrt ordering of start and end points within it.

That's a fair point. I'll remove it.

@nagisa Wouldn't this cause issues with Range<usize> or similar types where (correct me if I'm wrong) start is expected to be less than end?

nagisa · 2017-05-14T00:38:31Z

src/lib.rs

+//! *all* possible values of the type, just to a reasonable amount. For example,
+//! `rand::random::<f64>()` will never return `f64::NAN`, and will only return
+//! a value in the range [0, 1). `Option<u8>` will have a 1/2 chance of being,
+//! `None`, not 1/257.


It is widely accepted/known that floating point random numbers are generated in range 0 to 1. As for None, if you ignore the NonNull optimisation, it having a chance of ½ is uniform from the perspective of bit pattern, which is (discriminant, value).

clarfonthey · 2017-05-30T22:10:17Z

I've removed the note in lib.rs and made Range include backwards ranges. Let me know if you need anything before this is merged.

clarfonthey · 2017-06-08T02:45:58Z

Bumping this.

shanavas786 · 2017-07-19T09:47:04Z

src/rand_impls.rs

+
+impl Rand for Ipv4Addr {
+    #[inline]
+    fn rand<R: Rng>(rng: &mut R) -> Ipv4Addr {


rng.gen::<i8>() would prevent overflow

Ipv4Addr accepts u8s. This uses type inference.

Should this exclude certain special addresses perhaps, e.g. those ending in 0 or 255?

May be easiest and fastest to implement via looping on rejection.

dhardy · 2017-07-28T10:11:20Z

I'm not sure how many of these we want

@clarcharr Why? The only example I've come across so far where you might want random values of arbitrary types is fuzzing, but as I pointed out elsewhere this is very unlikely to generate certain special values that should probably be tested, e.g. 0, and may not give a sensible distribution for some types (e.g. Option<u32> would be None half the time in the current implementation, which may or may not be appropriate). Actually, for fuzzing, the current generator yielding floats in the range [0, 1) is probably not appropriate. And as dbaupp pointed out, the current implementation for char will yield unassigned codepoints 3/4 of the time.

Personally I see no use for these "default generators". If there is some use, e.g. fuzzing, then a specific distribution could be used instead (rand::distributions::Fuzzing, or probably better, an implementation in a fuzzing crate). If you think otherwise, please add your thoughts to the crate evaluation thread.

clarfonthey · 2017-07-30T01:50:10Z

@dhardy the main reason why I was thinking these were useful was for derive(Rand) and potentially generics over Rand. If it's not useful then the PR can be closed, but I mostly put these out there to see what the opinion on it was.

dhardy · 2017-07-31T09:25:38Z

@clarcharr thanks for your input. It's not for me to decide whether to close this PR.

Some of these like PhantomData are fair enough, but for others like Ordering — if you used derive(Rand) with this, would you necessarily want a 1/3 chance of Ordering::Equal? Perhaps for some uses the exact distribution doesn't matter, I don't know (my experience is with stochastic simulations, where distribution very much does matter, and the required distribution isn't always the same either).

clarfonthey · 2017-10-05T04:18:23Z

Bump; is this PR desired or should I just close it for now? I'm willing to update it based upon feedback, but I'm not sure what should be changed/removed at the moment.

dhardy

Well, some stuff like Wrapping seems like it should be added IMO. Some of the distributions seem weird and some of those ranges could be huge (so one can't just iterate over a random range). Stuff like Box which allocates... well, I can't see a real reason why it shouldn't be there. Adding extra features to Config.toml I'm not keen on.

But mostly I'll just say: patience! The rand refactor stuff will take a while, and there's not so much point in finalising this bit yet, although there's also no real reason it can't go ahead I guess (with the caveat that it may have to change later).

dhardy · 2017-10-05T05:28:39Z

src/rand_impls.rs

+impl Rand for Duration {
+    #[inline]
+    fn rand<R: Rng>(rng: &mut R) -> Duration {
+        Duration::new(rng.gen(), rng.gen_range(0, 1_000_000_000))


I would suggest using an exponential distribution here

I consider Duration to be very similar to any other integer type, and so uniform probability makes the most sense to me.

How often are you working with hundred million year durations?

@clarcharr that's a null argument considering there are many things people might want to do with random integers (virtual dice, randomised network delays, random sorting, random selection from a list (e.g. a set of Unicode glyphs, Poisson distributions) and probably none of them involve uniform sampling from a fixed range not specific to this use-case.

I suggested exponential here because that, or sampling from a short arbitrary range (e.g. 0 to 1 minutes) are about the only things that make any sense whatsoever when you don't know the target use.

dhardy · 2017-10-05T05:30:09Z

src/rand_impls.rs

+        match rng.gen_range(0, 3) {
+            0 => Ordering::Less,
+            1 => Ordering::Equal,
+            2 => Ordering::Greater,


1/3 chance of each ordering may be okay, but it's arbitrary and even weird. Not really sure; it's not worse than any other option but may still not be what's wanted. Still, we already have Option..

Fair! I was considering this for the sake of testing, and IMHO each should be equally likely to be tested.

dhardy · 2017-10-05T05:31:16Z

src/rand_impls.rs

+impl<T: Rand + PartialOrd> Rand for Range<T> {
+    #[inline]
+    fn rand<R: Rng>(rng: &mut R) -> Range<T> {
+        Range { start: rng.gen(), end: rng.gen() }


Don't we need to ensure end ≥ start ?

See previous comments from @nagisa.

dhardy · 2017-10-05T05:37:00Z

src/rand_impls.rs

+
+impl Rand for Ipv4Addr {
+    #[inline]
+    fn rand<R: Rng>(rng: &mut R) -> Ipv4Addr {


Should this exclude certain special addresses perhaps, e.g. those ending in 0 or 255?

May be easiest and fastest to implement via looping on rejection.

dhardy · 2017-10-05T05:42:35Z

src/rand_impls.rs

+    #[inline]
+    fn rand<R: Rng>(rng: &mut R) -> Ipv6Addr {
+        Ipv6Addr::new(rng.gen(), rng.gen(), rng.gen(), rng.gen(),
+                      rng.gen(), rng.gen(), rng.gen(), rng.gen())


Also, should any addresses be omitted here? No idea.

They're all valid addresses, even though many are not routable. So, IMHO this is correct.

That's the problem with general implementations like this: you have no idea how they will be used. Given that special addresses are very rare and therefore unlikely to come up by accident, it seems sensible to specify that they won't come up at all.

dhardy · 2017-10-05T05:47:07Z

src/rand_impls.rs

+}
+
+// TODO: remove Copy
+impl<T: Copy + Rand> Rand for Cell<T> {


I doubt you'll ever be able to remove Copy. So remove the TODO?

Actually, doing so only requires a minimum rustc bump.

dhardy · 2017-10-05T05:48:10Z

Cargo.toml

-nightly = ["i128_support"]
+bound = []         # std::collections is unstable in 1.0.0
+duration = []      # Duration is unstable in 1.0.0
+ip_addr = []       # IpAddr is unstable in 1.0.0


I'm not so keen on adding a bunch of conditional features here

Agreed! These should be added properly with a rustc version bump.

dhardy · 2018-02-10T14:39:18Z

All those interested: should we remove rand_derive? Comments in dhardy#83 please.

pitdicker · 2018-03-06T12:24:19Z

Now that Rand and rand-derive are deprecated, it seems to me almost all those impls for Rand do not make sense anymore. One that may still be desired are those for Wrapping<T>. Also Duration, but that is probably much better served by a range #82.

@clarcharr I know there have been many months while your PR has been sitting here. But now that we have a clear idea of the future of the Rand trait, would you still be willing to make the changes?

dhardy · 2018-03-06T12:46:06Z

Agreed. If we still had rand_derive then the PhantomData and Box impls would also make sense, but without that they are pointless. This Duration impl I don't like. #82 and #168 cover the remaining extensions, so I'm going to close this PR.

nagisa reviewed May 14, 2017

View reviewed changes

Add tonnes of impls for Rand.

136adb5

alexcrichton added the F-new-int Functionality: new, within Rand label Jun 14, 2017

shanavas786 reviewed Jul 19, 2017

View reviewed changes

dhardy reviewed Oct 5, 2017

View reviewed changes

dhardy mentioned this pull request Feb 10, 2018

Fate of the Rand trait dhardy/rand#83

Closed

dhardy closed this Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tonnes of impls for Rand. #148

Add tonnes of impls for Rand. #148

clarfonthey commented May 13, 2017 •

edited

Loading

nagisa May 14, 2017

clarfonthey May 14, 2017

nvzqz Jul 19, 2017

nagisa May 14, 2017

clarfonthey commented May 30, 2017

clarfonthey commented Jun 8, 2017

shanavas786 Jul 19, 2017

clarfonthey Jul 20, 2017

dhardy Oct 5, 2017

dhardy commented Jul 28, 2017 •

edited

Loading

clarfonthey commented Jul 30, 2017

dhardy commented Jul 31, 2017 •

edited

Loading

clarfonthey commented Oct 5, 2017

dhardy left a comment

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

sfackler Oct 8, 2017

dhardy Oct 9, 2017

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

dhardy Oct 5, 2017

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

dhardy Oct 9, 2017 •

edited

Loading

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

dhardy Oct 5, 2017

clarfonthey Oct 8, 2017

dhardy commented Feb 10, 2018

pitdicker commented Mar 6, 2018 •

edited

Loading

dhardy commented Mar 6, 2018

Add tonnes of impls for Rand. #148

Add tonnes of impls for Rand. #148

Conversation

clarfonthey commented May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented May 30, 2017

clarfonthey commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Jul 28, 2017 • edited Loading

clarfonthey commented Jul 30, 2017

dhardy commented Jul 31, 2017 • edited Loading

clarfonthey commented Oct 5, 2017

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Feb 10, 2018

pitdicker commented Mar 6, 2018 • edited Loading

dhardy commented Mar 6, 2018

clarfonthey commented May 13, 2017 •

edited

Loading

dhardy commented Jul 28, 2017 •

edited

Loading

dhardy commented Jul 31, 2017 •

edited

Loading

dhardy Oct 9, 2017 •

edited

Loading

pitdicker commented Mar 6, 2018 •

edited

Loading