Repository navigation
Import xoshiro crate - #642
Conversation
* This replaces the old xoshiro benchmarks. * rand_xoshiro now uses the correct version of rand_core.
|
Thanks. Might take me a couple of days to review. We can update the doc and publish |
|
Yeah, this is not urgent. |
dhardy
left a comment
dhardy
left a comment
There was a problem hiding this comment.
A couple of annoying little points here...
but other than the API doc I see no real reason this should be in the same repo. Maybe we should revive small-rngs or make a new rngs? The same is true for rand_xorshift, _isaac and _chacha once the deprecated re-exports are removed.
The advantages are faster/simpler CI, less stuff in this repo, and a separate issue tracker. I think it would be worth it. Shall I import your existing repo, move the code to a sub-directory, and publish as rust-random/rngs?
| - cargo test --manifest-path rand_isaac/Cargo.toml --features=serde1 | ||
| # TODO: cannot test rand_pcg due to explicit dependency on i128 | ||
| - cargo test --manifest-path rand_xorshift/Cargo.toml --features=serde1 | ||
| - cargo test --manifest-path rand_xoshiro/Cargo.toml |
There was a problem hiding this comment.
👍 but you should also add to utils/ci/script.sh and appveyor.yml. Yeah, it's a pain to have to configure CI in three places 😦
There was a problem hiding this comment.
Will do. Maybe we should automatically generate the scripts from from some declaration of crates.
There was a problem hiding this comment.
Maybe, though not all the scripts are identical (for annoying reasons). Not something I want to spend time solving.
| use rand_hc::{Hc128Rng, Hc128Core}; | ||
| use rand_pcg::{Lcg64Xsh32, Mcg128Xsl64}; | ||
| use rand_xorshift::XorShiftRng; | ||
| use rand_xoshiro::Xoshiro256StarStar; |
There was a problem hiding this comment.
I'd like all variants represented in the benchmarks... which does bring up the question of whether we should have them here.
There was a problem hiding this comment.
I think it is nice to compare the performance of all the generators in one place. This also reduces code duplication.
I'll add the other variants.
| [package] | ||
| name = "rand_xoshiro" | ||
| version = "0.1.0" # NB: When modifying, also modify html_root_url in lib.rs | ||
| authors = ["Vinzent Steinberg <Vinzent.Steinberg@gmail.com>"] |
There was a problem hiding this comment.
If you want to include this in the main repo, then I'd prefer the same convention as the other crates (though you can include yourself in addition if you like).
Didn't we decide against that because of problems with the docs? I don't see how that changed. I don't really care in which repository this lives, but I would prefer the links in the docs to work. |
|
Part for that reason, part because it was difficult having direct dependencies of |
| // David Stafford's | ||
| // (http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html) | ||
| // "Mix4" variant of the 64-bit finalizer in Austin Appleby's | ||
| // MurmurHash3 algorithm. |
There was a problem hiding this comment.
Can you explain what the link is? This algorithm is SplitMix. Isn't the point of this implementation to reproduce output from the official version?
There was a problem hiding this comment.
AFAIK, the official version only supports generating u64 natively. This is a more efficient variant for generating u32.
dhardy
left a comment
dhardy
left a comment
There was a problem hiding this comment.
Sorry for poor time estimate; apparently "a couple of days" can mean a month!
Looks good, but there are a lot of RNGs included. I specifically kept rand_pcg small because not many will be widely used and it keeps the dependencies of rand small.
|
AFAIK, this is likely not going to be a dependency of |
|
I guess there are no obvious candidates to drop. Apparently no-one else is opinionated on this, so lets just accept this as-is for now — although I am of the opinion that we should move this to another repository later (along with all other PRNGs not directly depended on by I'll leave this until tomorrow for any final comments. |
Import xoshiro crate
This is based on the xoshiro crate with minimal modifications.