-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
xoshiro265**
uniform random number generator
#1769
xoshiro265**
uniform random number generator
#1769
Conversation
…mplementation in C
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… into xoshiro_random_number_generator
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… into xoshiro_random_number_generator
As a sidenote: I'm testing the implementation against the reference implementation in https://prng.di.unimi.it/xoshiro256starstar.c by loading this extern "C" {
#include "xoshiro256starstar.c"
} A little bit odd perhaps?! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Would it be safer to use uint64_t instead of unsigned long long?
Haven't thought about it. 🤔 According to https://en.cppreference.com/w/cpp/language/types currently |
0d490e9
to
c47b1cb
Compare
ql/math/randomnumbers/splitmix64.hpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lballabio Little bit unsure about the SplitMix64
class here. Do we want it to be perhaps only an inner nested class in Xoshiro256StarStarUniformRng
?
Not sure if it is a fully prng? Do we want to use its functionality outside of Xoshiro256StarStarUniformRng
later on (separate class) or just for initialize xoshiro++ (inner nested)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, fine with me. But then it also needs to be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I wouldn't bother. It can be an inner class for the time being, or it can even be in an anonymous namespace in the cpp file.
s1_ = splitMix64.next(); | ||
s2_ = splitMix64.next(); | ||
s3_ = splitMix64.next(); | ||
} while (s0_ == 0 && s1_ == 0 && s2_ == 0 && s3_ == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is ever going to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree, it's very, very unlikely.
I have "stolen" the idea from c#
. Random number generator guys are probably extra paranoid careful. 😄
Personally I would keep it here as a reminder what theoretically might go wrong. But I leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd take it out. If the splitmix rng gives you 4 zeroes in a row, it's probably going to keep giving you zeroes.
@ralfkonrad should we finalize this? I can do the last changes if you don't mind. |
@lballabio Sorry, busy times plus Corona virus prevented making progress here. Do I have this weekend to finish it? Or do you want to ensure to get it into next release? Then please go ahead. |
Sorry to hear that—are you ok now? In any case, don't use your weekend for this, I'll do the changes :) |
Thanks @lballabio for picking this up. I would have probably over-engineered it in the approach I had in mind... 😃 I'm okay, but it took a while. Needed a lot of rest besides the regular work. |
Over the last couple of years the family of xoshiro / xoroshiro pseudorandom number generators has become popular and is being adopted in a couple of programming languages as the new default prng or at least as one of its optional ones. The main reason for this family is its performance, it is faster than the Mersenne twister.
Out of this family I have implemented
xoshiro265**
here which has a of period2^256-1
.On my machine this implementation is approx. twice as fast in returning a
double
and still 20% faster for theBoxMullerGaussian.next()
compared to Mersenne Twister.Windows benchmark
Ubuntu wsl benchmark