Move `XorShiftRng` to its own crate #557

vks · 2018-07-16T14:23:16Z

Also recommend StdRng rather than the deprecated IsaacRng.

The main motivation for this is that we can remove the generator from Rand and that it is still possible to reproduce the results.

Also recommend `StdRng` rather than the deprecated `IsaacRng`.

dhardy

Looks good, though CI has a few complaints.

dhardy · 2018-07-16T15:02:30Z

src/prng/mod.rs


 pub use self::chacha::ChaChaRng;
 pub use self::hc128::Hc128Rng;
-pub use self::xorshift::XorShiftRng;
+pub use ::rand_xorshift::XorShiftRng;


Ideally this should use deprecated::XorShiftRng. And then SmallRng will need to change the import path to avoid deprecation warnings.

Also the deprecation warning itself should suggest the new path

vks · 2018-07-16T17:27:08Z

I should mention that I decided against adding a `random` or `xorshift` keyword, because they are already in the description. This makes them redundant for search, but they could still be useful for listing crates by keyword. It is inconsistent with the keywords for `rand_isaac`.

…

On Mon, Jul 16, 2018, 17:03 Diggory Hardy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/prng/mod.rs <#557 (comment)> : > pub use self::chacha::ChaChaRng; pub use self::hc128::Hc128Rng; -pub use self::xorshift::XorShiftRng; +pub use ::rand_xorshift::XorShiftRng; Also the deprecation warning itself should suggest the new path — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#557 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACCtNavrSfVO68RZ3mCcHKR2WOGXPnpks5uHKs4gaJpZM4VROGc> .

dhardy · 2018-07-16T18:52:45Z

I didn't pay much attention to keywords... but is there a problem with having too many?

vks · 2018-07-18T08:28:46Z

is there a problem with having too many?

There is an upper limit of 5, but we are using less than that. Anyway, I made the rand_xorshift keywords consistent with what we have elsewhere.

vks · 2018-07-18T11:24:18Z

WASM fails as expected, but I can't see other failures.

dhardy · 2018-07-18T13:03:38Z

There are some broken documentation links; check the bottom: https://travis-ci.org/rust-lang-nursery/rand/jobs/405253376

This is a different issue (can you investigate?): https://travis-ci.org/rust-lang-nursery/rand/jobs/405253378

Also there was a warning about unused #[macro_use].

Be nice if you can investigate; I have very little time this week.

vks · 2018-07-18T13:31:08Z

I fixed the broken links.
The macro warning can be fixed, but we probably will have to increase the required serde version. (There is no warning with an old version, and I suspect compilation will fail for those versions if I fix the warning.)
Not sure about the WASM failures.

vks · 2018-07-18T14:18:32Z

The WASM failure happens on master as well, so I think it is orthogonal to this PR.

dhardy · 2018-07-18T15:58:07Z

Yes, sorry, the WASM failure is a known issue at the moment: koute/cargo-web#116

I guess then we should just ignore the warning for now.

pitdicker

👍. Left two small comments.

What should the version number of these crates be? It is pretty much stable, but the depends on rand_core < 1.0.

pitdicker · 2018-07-20T06:22:35Z

rand_xorshift/Cargo.toml

+appveyor = { repository = "alexcrichton/rand" }
+
+[features]
+serde1 = ["serde", "serde_derive"] # enables serde for BlockRng wrapper


I think this only needs serde_derive? And the comment is old.

I don't think so. Actually, it seems like newer versions of serde don't need serde_derive anymore. I'll remove the comments though.

pitdicker · 2018-07-20T06:23:22Z

rand_xorshift/src/lib.rs

+
+mod xorshift;
+
+pub use self::xorshift::XorShiftRng;


Any reason xorshift.rs can't be moved in with lib.rs?

It could be. I'm just mirroring what was done before. Why would you prefer this?

It doesn't matter much, but I've seen more tiny crates like this have everything in just a single file.

pitdicker · 2018-07-20T06:43:12Z

Cargo.toml


 [dependencies]
 rand_core = { path = "rand_core", version = "0.2", default-features = false }
 # only for deprecations and benches:
 rand_isaac = { path = "rand_isaac", version = "0.1", default-features = false }
+rand_xorshift = { path = "rand_xorshift", version = "0.1", default-features = false }


It doesn't have default features (neither does rand_isaac).

I'll remove it.

vks · 2018-07-20T13:46:23Z

I realized the serde tests were broken in rand_isaac and rand_xorshift and fixed them.

pitdicker · 2018-07-20T13:52:56Z

rand_isaac/Cargo.toml

@@ -18,9 +18,15 @@ travis-ci = { repository = "rust-lang-nursery/rand" }
 appveyor = { repository = "alexcrichton/rand" }

 [features]
-serde1 = ["serde", "serde_derive"] # enables serde for BlockRng wrapper
+serde1 = ["serde", "serde_derive", "rand_core/serde1"]
+std = []


I don't follow why you added an extra std feature... Or does testing serialization need std?

Yes, exactly. The test uses BufWriter, BufReader and bincode, all requiring std.

What do you think about only enabling std when testing, or even only when testing in combination with serde? Than we don't need the otherwise useless feature flag.

Seems a bit fragile. What if you want to test Serde without std? Anyway, this is not possible AFAIK.

pitdicker · 2018-07-20T13:56:55Z

src/lib.rs

@@ -237,7 +237,7 @@
 #[cfg(feature="std")] extern crate std as core;
 #[cfg(all(feature = "alloc", not(feature="std")))] extern crate alloc;

-#[cfg(test)] #[cfg(feature="serde1")] extern crate bincode;
+#[cfg(all(feature="serde1", test))] extern crate bincode;


This comment does not really belong to this PR, but does rand still need bincode as dependency, or do all tests now live in external crates?

Actually, after this PR rand does not depend directly on serde or bincode anymore. I removed them.

We might want to implement Serde support for StdRngand SmallRng. The serialization stability guarantees could be the same as for value stability.

pitdicker · 2018-07-20T13:59:39Z

I realized the serde tests were broken in rand_isaac and rand_xorshift and fixed them.

Does the CI run tests for rand_isaac and rand_xorshift?

vks · 2018-07-20T14:17:23Z

Does the CI run tests for rand_isaac and rand_xorshift?

Yes, but only on nightly due to the cargo test --all --benches command. (Seems like an accident TBH.) However, this does not include the serde tests, because they require features.

I'll push a commit that enable more tests on more targets.

Before, there Serde tests where not running.

pitdicker · 2018-07-20T14:52:22Z

We might want to implement Serde support for StdRng and SmallRng. The serialization stability guarantees could be the same as for value stability.

Thanks to having a SeedableRng implementation StdRng and SmallRng can give reproducible output, provided the version of rand and the platform remain the same. We don't make stronger guarantees than those, so we are free to chose a more optimal algorithm in future versions or on certain platforms.

I would expect serialization to give stronger reproducibility guarantees than that. That was the reason we didn't implement serialization for those, and it still seems like the right choice to me.

vks · 2018-07-20T15:53:44Z

I would expect serialization to give stronger reproducibility guarantees than that.

Why? I would expect that breaking reproducibility also breaks serialization. And why would you want serialization without reproducibility?

Anyway, I agree the conservative choice is to only implement serialization for named RNGs for now.

I think this PR is ready now?

pitdicker · 2018-07-20T18:12:09Z

Why? I would expect that breaking reproducibility also breaks serialization. And why would you want serialization without reproducibility?

We are saying the same thing 😄.

Seems a bit fragile. What if you want to test Serde without std? Anyway, this is not possible AFAIK.

Testing serialization without std does not seem worthwile for us (it is a minor feature). Removing the std feature is possible, would you cherry-pick my commit? https://github.com/pitdicker/rand/commits/xorshift-crate

dhardy · 2018-07-23T07:47:48Z

What should the version number of these crates be? It is pretty much stable, but the depends on rand_core < 1.0.

Starting fresh with 0.1 makes sense to me. As you point out, it's not reasonable to use 1.0 until after all dependencies are stable, and IMO this should only happen after the library has existed for some time with no significant changes.

vks · 2018-07-23T11:09:56Z

@pitdicker

Removing the std feature is possible, would you cherry-pick my commit?

Your branch does not work, the serde tests are never executed. Applying the following patche fixes this:

diff --git a/rand_isaac/src/isaac.rs b/rand_isaac/src/isaac.rs
index 6a8121b..a1bd378 100644
--- a/rand_isaac/src/isaac.rs
+++ b/rand_isaac/src/isaac.rs
@@ -453,7 +453,7 @@ mod test {
     }
 
     #[test]
-    #[cfg(all(feature="serde1", feature="std"))]
+    #[cfg(all(feature="serde1"))]
     fn test_isaac_serde() {
         use bincode;
         use std::io::{BufWriter, BufReader};
diff --git a/rand_isaac/src/isaac64.rs b/rand_isaac/src/isaac64.rs
index b77f3bb..30c494a 100644
--- a/rand_isaac/src/isaac64.rs
+++ b/rand_isaac/src/isaac64.rs
@@ -445,7 +445,7 @@ mod test {
     }
 
     #[test]
-    #[cfg(all(feature="serde1", feature="std"))]
+    #[cfg(all(feature="serde1"))]
     fn test_isaac64_serde() {
         use bincode;
         use std::io::{BufWriter, BufReader};
diff --git a/rand_xorshift/src/xorshift.rs b/rand_xorshift/src/xorshift.rs
index 8a93e9a..cf12296 100644
--- a/rand_xorshift/src/xorshift.rs
+++ b/rand_xorshift/src/xorshift.rs
@@ -178,7 +178,7 @@ mod tests {
         }
     }
 
-    #[cfg(all(feature="serde1", feature="std"))]
+    #[cfg(all(feature="serde1"))]
     #[test]
     fn test_xorshift_serde() {
         use bincode;

However, this makes cargo test --features serde1 fail on no_std targets. I guess this is fine, because in this case the serde1 feature would not be tested in any case.

Note that you can push to my branch due to your collaborator privileges.

pitdicker · 2018-07-23T11:21:40Z

Thank you!

@dhardy You already gave your approval, but there have been changes. Shall I merge?

dhardy

Reviewed again. With some doc tweaks I'm happy to have this merged, though it would be nice to fix the tests first.

@pitdicker does the Travis changes clash with #563?

dhardy · 2018-07-24T10:56:43Z

benches/distributions.rs

@@ -4,14 +4,15 @@

 extern crate test;
 extern crate rand;
+extern crate rand_xorshift;


Maybe we should just use SmallRng in this benchmark?

dhardy · 2018-07-24T11:00:03Z

rand_isaac/Cargo.toml

@@ -18,9 +18,14 @@ travis-ci = { repository = "rust-lang-nursery/rand" }
 appveyor = { repository = "alexcrichton/rand" }

 [features]
-serde1 = ["serde", "serde_derive"] # enables serde for BlockRng wrapper
+serde1 = ["serde", "serde_derive", "rand_core/serde1"]


Feature serde1 implicitly implies std — should probably be documented with a comment here and in the README

Feature serde1 implicitly implies std

What do you mean? We don't require std for the serde1 feature, and Serde works without std. We only need std for the tests.

Oh, sorry. I mis-understood the double-negative conditional no_std attribute.

dhardy · 2018-07-24T11:00:26Z

rand_isaac/src/lib.rs

@@ -18,14 +18,17 @@
 #![deny(missing_debug_implementations)]
 #![doc(test(attr(allow(unused_variables), deny(warnings))))]

-#![no_std]
+#![cfg_attr(not(all(feature="serde1", test)), no_std)]


Makes sense, but is surprising

The only alternative I can see would be making our tests work with no_std.

dhardy · 2018-07-24T11:04:25Z

rand_xorshift/Cargo.toml

+appveyor = { repository = "alexcrichton/rand" }
+
+[features]
+serde1 = ["serde", "serde_derive"]


Same as for rand_isaac

vks · 2018-07-24T11:29:13Z

it would be nice to fix the tests first.

They are failing on master as well, I don't think they are related to this PR.

does the Travis changes clash with #563?

IIRC, #563 includes the changes here, but commented out.

dhardy · 2018-07-24T11:33:21Z

They are failing on master as well, I don't think they are related to this PR.

Hence "nice", not "required". I didn't really want to look all through those AppVeyor logs.

vks · 2018-07-24T11:37:41Z

Hence "nice", not "required". I didn't really want to look all through those AppVeyor logs.

AFAIK, #565 fixes it but is blocked on packed_simd upstream changes.

Move XorShiftRng to its own crate

a253a80

Also recommend `StdRng` rather than the deprecated `IsaacRng`.

dhardy approved these changes Jul 16, 2018

View reviewed changes

dhardy reviewed Jul 16, 2018

View reviewed changes

vks added 2 commits July 18, 2018 10:14

Use keywords consistent with other Rand crates

a385938

Make sure XorShiftRng is deprecated

2cf9acb

vks added 2 commits July 18, 2018 10:34

Fix benchmarks

9b069fe

Fix deprecation warnings in benchmarks

2847c8a

vks added 2 commits July 18, 2018 15:19

Fix broken links in docs

ecc6e66

Fix compilation warning with serde

a3ac8d3

dhardy added the D-review Do: needs review label Jul 18, 2018

pitdicker reviewed Jul 20, 2018

View reviewed changes

vks added 2 commits July 20, 2018 15:44

Fix serde tests for rand_isaac and rand_xorshift

2a658d9

rand_xorshift: Add README

dac6227

pitdicker reviewed Jul 20, 2018

View reviewed changes

vks added 2 commits July 20, 2018 16:17

Enable more CI tests

ecd1867

Before, there Serde tests where not running.

The rand crate does not depend on serde or bincode anymore

e7b17dd

Don't add std feature to rand_isaac and rand_xorshift

29a9d1d

Fix serde tests

cb275bc

dhardy reviewed Jul 24, 2018

View reviewed changes

Simplify attributes

d2e7378

Prefer SmallRng in benchmarks

b846c30

dhardy merged commit f8fd39d into rust-random:master Jul 24, 2018

vks deleted the xorshift-crate branch July 24, 2018 14:13

This was referenced Jul 31, 2018

Tracker: 0.6 release #520

Closed

Move PRNG algorithms out of Rand #431

Closed

sync-by-unito bot mentioned this pull request Dec 15, 2020

Bump rand_core from 0.5.1 to 0.6.0 ProvableHQ/leo#503

Merged

Move XorShiftRng to its own crate #557

Move XorShiftRng to its own crate #557

Conversation

vks commented Jul 16, 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

vks commented Jul 16, 2018 via email

dhardy commented Jul 16, 2018

vks commented Jul 18, 2018

vks commented Jul 18, 2018

dhardy commented Jul 18, 2018

vks commented Jul 18, 2018

vks commented Jul 18, 2018

dhardy commented Jul 18, 2018

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vks Jul 20, 2018 • 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

Choose a reason for hiding this comment

vks commented Jul 20, 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

vks Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

pitdicker commented Jul 20, 2018

vks commented Jul 20, 2018

pitdicker commented Jul 20, 2018 • edited Loading

vks commented Jul 20, 2018

pitdicker commented Jul 20, 2018

dhardy commented Jul 23, 2018

vks commented Jul 23, 2018

pitdicker commented Jul 23, 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

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

vks commented Jul 24, 2018

dhardy commented Jul 24, 2018

vks commented Jul 24, 2018

Move `XorShiftRng` to its own crate #557

Move `XorShiftRng` to its own crate #557

vks Jul 20, 2018 •

edited

Loading

vks Jul 20, 2018 •

edited

Loading

pitdicker commented Jul 20, 2018 •

edited

Loading