Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upImport xoshiro crate #642
Conversation
vks
added some commits
Nov 14, 2018
vks
requested a review
from
dhardy
Nov 14, 2018
vks
added some commits
Nov 14, 2018
This comment has been minimized.
This comment has been minimized.
|
Thanks. Might take me a couple of days to review. We can update the doc and publish |
This comment has been minimized.
This comment has been minimized.
|
Yeah, this is not urgent. |
dhardy
reviewed
Nov 20, 2018
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?
| @@ -71,6 +71,7 @@ matrix: | |||
| - 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 | |||
This comment has been minimized.
This comment has been minimized.
dhardy
Nov 20, 2018
Member
utils/ci/script.sh and appveyor.yml. Yeah, it's a pain to have to configure CI in three places
This comment has been minimized.
This comment has been minimized.
vks
Nov 21, 2018
Collaborator
Will do. Maybe we should automatically generate the scripts from from some declaration of crates.
This comment has been minimized.
This comment has been minimized.
dhardy
Nov 21, 2018
Member
Maybe, though not all the scripts are identical (for annoying reasons). Not something I want to spend time solving.
| @@ -30,6 +31,7 @@ use rand_chacha::ChaChaRng; | |||
| use rand_hc::{Hc128Rng, Hc128Core}; | |||
| use rand_pcg::{Lcg64Xsh32, Mcg128Xsl64}; | |||
| use rand_xorshift::XorShiftRng; | |||
| use rand_xoshiro::Xoshiro256StarStar; | |||
This comment has been minimized.
This comment has been minimized.
dhardy
Nov 20, 2018
Member
I'd like all variants represented in the benchmarks... which does bring up the question of whether we should have them here.
This comment has been minimized.
This comment has been minimized.
vks
Nov 21, 2018
Collaborator
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>"] |
This comment has been minimized.
This comment has been minimized.
dhardy
Nov 20, 2018
Member
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).
This comment has been minimized.
This comment has been minimized.
rand_xoshiro/LICENSE-MIT Outdated
vks
added some commits
Nov 21, 2018
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
|
Part for that reason, part because it was difficult having direct dependencies of |
rand_xoshiro/src/splitmix64.rs Outdated
| // 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. |
This comment has been minimized.
This comment has been minimized.
dhardy
Dec 14, 2018
Member
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?
This comment has been minimized.
This comment has been minimized.
vks
Dec 17, 2018
Collaborator
AFAIK, the official version only supports generating u64 natively. This is a more efficient variant for generating u32.
rand_xoshiro/src/xoroshiro128plus.rs Outdated
rand_xoshiro/src/xoroshiro64star.rs Outdated
rand_xoshiro/src/xoshiro128plus.rs Outdated
rand_xoshiro/src/xoshiro256plus.rs Outdated
rand_xoshiro/src/xoshiro512plus.rs Outdated
dhardy
reviewed
Dec 14, 2018
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.
This comment has been minimized.
This comment has been minimized.
|
AFAIK, this is likely not going to be a dependency of |
This comment has been minimized.
This comment has been minimized.
|
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. |
vks commentedNov 14, 2018
This is based on the xoshiro crate with minimal modifications.