Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented XoShiro128 psuedo random number generator. #428

Closed
wants to merge 2 commits into from
Closed

Implemented XoShiro128 psuedo random number generator. #428

wants to merge 2 commits into from

Conversation

thelearnerofcode
Copy link

Still have to add tests and documentation, but this is a good start.

See here for more info: http://xoshiro.di.unimi.it/.

@pitdicker
Copy link
Contributor

Hi @thelearnerofcode. Thank you for the PR!

We have plans to move all specific PRNG implementations out of Rand, so I don't think adding any xoshiro variants is what we want at the moment. And we have a couple of other ones higher up the list in terms of priority, which are already postponed until after Rand 0.5 gets released.

I have not read the details of this new RNG yet, it is only released very recently. Maybe it should prove itself for a couple of months. And I am curious how it holds up under TestU01 and PractRand, where Xoroshiro128+ performed worse than what was advertised.

@thelearnerofcode
Copy link
Author

Ok! Should I close the PR?

@pitdicker
Copy link
Contributor

Maybe for now, but you can also leave it open for some discussion. I am sure @vks is interested, he mentioned the same RNG yesterday in dhardy#60 (comment), and is the author of the xoroshiro crate, which implements more work by Vigna.

@vks
Copy link
Collaborator

vks commented May 5, 2018

I worked on a crate for xoshiro128** yesterday. It can be partially vectorized with SIMD, resulting in a nice speedup.

@dhardy
Copy link
Member

dhardy commented May 6, 2018

Cool! Sounds to me like we should start discussing how we want to present extra RNGs to users (dhardy#58) soon (in fact, if someone wants to open a new issue with a proposal, go ahead; I think companion crates are probably the best option but don't know how many). However, I don't think we should merge anything before the 0.5 release (very soon, hopefully).

This PR looks very nice but I don't think we should merge new PRNGs before sorting out the above. Also since this is part of a family of new PRNGs I don't know if we should pick names like Xoshiro128SS.

PS nice SIMD work @vks. But how are we supposed to differentiate between [u64; 2] and [u32; 4] versions of Xoshiro128?

@pitdicker
Copy link
Contributor

Sounds to me like we should start discussing how we want to present extra RNGs to users (dhardy#58) soon

Opened #431.

@dhardy dhardy added T-RNG P-postpone Waiting on something else labels May 7, 2018
@vks
Copy link
Collaborator

vks commented May 7, 2018

@dhardy Not sure what you mean. There is only one xoshiro128** and xoshiro128+ version, both use [u32; 4]. Only xoroshiro128 uses [u64; 2].

@vks
Copy link
Collaborator

vks commented May 7, 2018

This PR does not implement xoshiro128** as claimed by the code, but rather xoroshiro128**!

@dhardy
Copy link
Member

dhardy commented May 28, 2018

Lets close this for now. We don't want it in its current form, and if we do decide to add this PRNG we can refer back to this PR.

@dhardy dhardy closed this May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-postpone Waiting on something else
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants