Undeprecate random #422

pitdicker · 2018-05-01T13:46:38Z

Undeprecate random() as discussed in #289.

Mostly with this PR I took a stab at making the documentation a bit more friendly for people who don't yet know much about generating random numbers. The documentation did become a bit much I am afraid...

LukasKalbertodt

Very nice! I like it a lot. However, I only read the additions and didn't take a look at what you removed. I also really don't know this library, so take my opinion with a grain of salt :P

LukasKalbertodt · 2018-05-01T13:56:43Z

src/lib.rs

+//! - [`gen`] generates a random value that fits the type.
+//!   For example `let val: u16 = rng.gen()` will just fill an `u16` with 16
+//!   random bits. But for floats the value will be generated between 0 and 1;
+//!   which is more usable than any value between 0 and infinity.


To be pedantic: to fill a float with bits does not necessarily result in a number between 0 and infinity. Maybe rather say "is more usable than a completely random float value (including NaN and infinity)" or something like that...

a random value appropriate for the type.

.. = rng.gen() produces a random value between 0 and std::u16::MAX,

I think better to say more useful than usable

Generating a value between 0 and infinity is not possible with a uniform distribution, which I think is the bigger issue. Maybe we should also mention that the bits are uniformly sampled.

LukasKalbertodt · 2018-05-01T13:58:35Z

src/lib.rs

+//! - [`StdRng`]. In addition to the output being indiscernible from true
+//!   randomness, this PRNG is unpredictable, even if you know the
+//!   implementation. This requirement makes it slower and/or use more memory
+//!   however.


Maybe mention that the property of "unpredictability" is important for cryptography?

We could mention that unpredictable PRNGs are usually called called "cryptographically secure PRNGs".

Did you actually define before what the acronym PRGN means?

LukasKalbertodt · 2018-05-01T14:00:01Z

src/lib.rs

+//!
+//! [`ThreadRng`] abstracts away most of these issues. It uses [`StdRng`], which
+//! is always a safe choice. Seeding happens automatically on first use. The
+//! PRNG its state is stored in thread local storage, so no need to plumb the


Typo: "The PRNG its state" -> "The PRNG's state"

LukasKalbertodt · 2018-05-01T14:02:03Z

src/lib.rs

+//! generate floating point values in the (0, 1) interval.
+//!
+//! For floating point values you may want to use probability distributions.
+//! For example time of decay is often modelled with an exponential


, after "example"... I think?

LukasKalbertodt · 2018-05-01T14:03:43Z

src/lib.rs

+//!
+//! - [`Rng::shuffle`] can be used to shuffle slices.
+//! - [`Rng::choose`] to pick one element at random from a slice, and the
+//!   functions in the [`seq`] the pick multiple.


Typo: "in the [seq] module to pick multiple"

dhardy

Sorry, didn't review the entire thing...

... but I have two main issues with this:

The intro in the API is already long; that's why I moved those examples. Scrolling past the same text every time to get to the API is a nuisance.
While it makes sense to have some documentation aimed at people less familiar with random number generation, there are also technical users, who may be scared away by highly informal doc like this. So either we need to find a compromise or find a way of moving some of the doc elsewhere.

dhardy · 2018-05-01T14:22:21Z

src/lib.rs

+//!
+//! - a random number generator (RNG);
+//! - some function to transform the 'bag of bits' from the RNG to a value you
+//!   can use.


Worth mentioning that we call this a distribution here?

dhardy · 2018-05-01T14:34:49Z

src/lib.rs

+//!
+//! There are many kinds of RNGs, with different trade-offs. Rand comes with a
+//! default, [`thread_rng`]. The numbers it generates are suitable to use in all
+//! situations.


Perhaps say:

There are many kinds of RNGs, but we are still able to pick a good all-use default which is fast (amortised performance), has good quality (secure and unbiased), and to automatically make a thread-local instance on first use; we call this thread_rng.

I like it! Not sure about "amortised performance", I would probably expand or remove it. (BTW, are we using British or American spelling?) I think "unbiased" could be replaced with "unpredictable".

dhardy · 2018-05-01T14:40:16Z

src/lib.rs

+//! - [`gen`] generates a random value that fits the type.
+//!   For example `let val: u16 = rng.gen()` will just fill an `u16` with 16
+//!   random bits. But for floats the value will be generated between 0 and 1;
+//!   which is more usable than any value between 0 and infinity.


a random value appropriate for the type.

.. = rng.gen() produces a random value between 0 and std::u16::MAX,

I think better to say more useful than usable

dhardy · 2018-05-01T14:41:51Z

src/lib.rs

+//!   which is more usable than any value between 0 and infinity.
+//! - [`gen_range`] samples from a specific range of values;
+//! - [`sample`], for all situations where you need something slightly more
+//!   complex.


Maybe add fill to this list?

dhardy · 2018-05-01T14:44:59Z

src/lib.rs

+//! of them, by measuring different kinds of noice/jitter in the hardware.
+//!
+//! What is always used instead is a psuedo-random number generator. This is a
+//! clever algorithm that can produce an infinite stream of random numbers from


psuedo-random or apparently random; the values are not actually random (by the usual definitions)

Typo: psuedo -> pseudo

Ok, I'll change it here. But I don't think we should get too pedantic and call it pseudo-random everywhere. It is theoretically right, but not an all that interesting distinction.

dhardy · 2018-05-01T14:45:45Z

src/lib.rs

+//! What is always used instead is a psuedo-random number generator. This is a
+//! clever algorithm that can produce an infinite stream of random numbers from
+//! a small seed. There exist algorithms that produce random numbers that are in
+//! no measurable way any less random than pulling values 'out of thin air'.


Untrue: if you know the seed, you can reproduce

Pedantically speaking, even cryptographic RNGs may be distinguished from true randomness. The definition says it cannot be done in polynomial time. So I would suggest to say "in no practical way" and add the caveat "without knowing the seed".

dhardy · 2018-05-01T14:50:13Z

src/lib.rs

+//!
+//! - [`thread_rng`] is what you often want to use.
+//! - If you want more control or better performance, use [`StdRng`] or
+//!   [`SmallRng`].


SmallRng is not always faster — maybe emphasise benchmarking? Also for more control one should specify the algorithm. I think the main use-case for specifying StdRng is going to be in generic code.

pitdicker · 2018-05-01T15:09:28Z

The intro in the API is already long; that's why I moved those examples. Scrolling past the same text every time to get to the API is a nuisance.

While it makes sense to have some documentation aimed at people less familiar with random number generation, there are also technical users, who may be scared away by highly informal doc like this. So either we need to find a compromise or find a way of moving some of the doc elsewhere.

Yes. I am not sure what would be a better place. Maybe we can hope this is temporary, and doxidize gets available soon?

vks · 2018-05-01T15:21:00Z

src/lib.rs

+//! # Getting random values
+//!
+//! To quickly get you started, the easiest and most high-level way to just get
+//! some random value is to use [`random()`].


I think "some random value" is too vague. Please mention that it is from a uniform distribution.

I hoped to avoid terminology until we get below

As soon as you need something slightly more specific, like a random value in some range, you have to know a bit more about how things work, which are discussed below.

vks · 2018-05-01T15:22:12Z

src/lib.rs

+//! ```
+//!
+//! This functionality is however very basic. As soon as you need something
+//! slightly more specific, like a random value in some range, you have to know


I would prefer "a random value sampled from a specific distribution". It's more precise.

The point of this documentation seems to be that it is not very precise. Which doesn't suit all of us...

I hope that is not the case... I thought the goal was to be beginner-friendly, maybe that is possible while being precise?

dhardy · 2018-05-01T15:38:58Z

Yes. I am not sure what would be a better place. Maybe we can hope this is temporary, and doxidize gets available soon?

For now we could simply link to Markdown files on GitHub — slightly ugly, but works. Or we could do something like add empty submodules just for extra documentation pages (very ugly in a different way).

vks · 2018-05-01T15:39:48Z

src/lib.rs

+//! - [`SmallRng`]. Uses little memory, and is very fast.
+//!   Note: The currently picked algorithm is not ideal, but in a future version
+//!   the values will for all practical purposes be statistically indiscernible
+//!   from true randomness.


I don't think this will ever be true for SmallRng. Either it is unpredictable and thus cryptographically secure, or we should not claim so. I suggest to not talk about predictability or to say that it is not guaranteed to be unpredictable.

"for all practical purposes be statistically indiscernible from true randomness" means something very different than "unpredictable and thus cryptographically secure".

Because the next lines says "In addition to the output being indiscernible from true randomness, this PRNG is unpredictable", I hope readers will understand that CSPRNGs set a higher standard and offer additional guarantees.

A "random" source is discernible from true randomness if it is predictable. There's scope for confusion here.

vks · 2018-05-01T15:46:12Z

src/thread_rng.rs

@@ -146,7 +143,7 @@ impl CryptoRng for ThreadRng {}
 /// If you're calling `random()` in a loop, caching the generator as in the
 /// following example can increase performance.
 ///
-/// ```
+/// ```rust


I think the rust is redundant. (However, I guess it does not hurt to be explicit.)

vks · 2018-05-01T15:47:30Z

src/thread_rng.rs

@@ -166,8 +163,7 @@ impl CryptoRng for ThreadRng {}
 /// ```
 ///
 /// [`thread_rng`]: fn.thread_rng.html
-/// [`Rand`]: trait.Rand.html
-#[deprecated(since="0.5.0", note="removed in favor of thread_rng().gen()")]
+/// [`Standard`]: distributions/struct.Standard.html


Maybe we should mention the disadvantages over using thread_rng().gen() here.

I don't think this is needed — there's already a note about caching the thread_rng handle above

pitdicker · 2018-05-02T06:40:40Z

I tried to make some of the documentation collapsible with some extra css, but the html Rustdoc outputs doesn't make that easy. Not ready to pull out js, I never serriously used that.

Maybe interesting for comparison, regex and log also have a pretty long introduction.

And there now also is a - just before the documentation to fold it.

pitdicker · 2018-05-02T06:43:58Z

Added cargo clean to Travis, because the dead links checker should fail but doesn't. Let's see if this works, but it may need a better solution.

dhardy · 2018-05-02T09:01:30Z

The documentation is already collapsible (click the [-] on the top-right). But IMO we should still try to limit length here.

dhardy

@pitdicker could you please separate out the first commit (un-deprecation) and move the other commits (about top-level doc) to a new PR?

dhardy · 2018-05-02T15:59:56Z

src/thread_rng.rs

@@ -166,8 +163,7 @@ impl CryptoRng for ThreadRng {}
 /// ```
 ///
 /// [`thread_rng`]: fn.thread_rng.html
-/// [`Rand`]: trait.Rand.html
-#[deprecated(since="0.5.0", note="removed in favor of thread_rng().gen()")]
+/// [`Standard`]: distributions/struct.Standard.html


I don't think this is needed — there's already a note about caching the thread_rng handle above

dhardy · 2018-05-02T16:01:49Z

src/thread_rng.rs

 /// Generates a random value using the thread-local random number generator.
 ///
 /// This is simply a shortcut for `thread_rng().gen()`. See [`thread_rng`] for
-/// documentation of the entropy source and [`Rand`] for documentation of
+/// documentation of the entropy source and [`Standard`] for documentation of


Add a quick mention of using thread_rng().gen_range(a, b) here? That (and maybe a link to Rng) is enough though; we don't need to go over everything here.

pitdicker · 2018-05-02T16:27:31Z

could you please separate out the first commit (un-deprecation) and move the other commits (about top-level doc) to a new PR?

Ok, done.

I am trying something slightly crazy in this branch: moving all *Rng things into an rngs module. WIP, but what do you think? It seems to both clean up our re-exports story and provide more room to spread out the documentation.

pitdicker · 2018-05-03T10:57:52Z

Now that this PR is tiny, is it ok to merge? (and tackle the documentation in the other PR?)

dhardy · 2018-05-03T11:59:19Z

It's good enough I guess though there are still outstanding comments.

pitdicker · 2018-05-03T12:12:03Z

I won't forget them, but would like to make the documentation changes part of #423.

Un-deprecate random()

a2a903c

LukasKalbertodt reviewed May 1, 2018

View reviewed changes

pitdicker mentioned this pull request May 1, 2018

Remove weak_rng and random? #289

Closed

3 tasks

dhardy reviewed May 1, 2018

View reviewed changes

vks reviewed May 1, 2018

View reviewed changes

dhardy reviewed May 2, 2018

View reviewed changes

pitdicker force-pushed the undeprecate_random branch from 189b4f6 to a2a903c Compare May 2, 2018 16:21

pitdicker mentioned this pull request May 2, 2018

Documentation #423

Merged

dhardy merged commit a8618f1 into rust-random:master May 3, 2018

pitdicker deleted the undeprecate_random branch May 3, 2018 14:59

Undeprecate random #422

Undeprecate random #422

Conversation

pitdicker commented May 1, 2018

LukasKalbertodt 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

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

pitdicker commented May 1, 2018

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 May 1, 2018

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

pitdicker commented May 2, 2018

pitdicker commented May 2, 2018

dhardy commented May 2, 2018

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

pitdicker commented May 2, 2018

pitdicker commented May 3, 2018

dhardy commented May 3, 2018

pitdicker commented May 3, 2018