lemire / testingRNG

Testing common random-number generators (RNG)
Apache License 2.0
172 stars 22 forks source link

splitmix64_stateless(x) not really splitmix64 w/ seed=x #7

Closed achan001 closed 6 years ago

achan001 commented 6 years ago

It will not affect PRNG testing, but initialize stateless version w/ seed x
actually mean initialize using splitmix64 with *seed = (x - 1) splitmix_step**

It might be better to have both version do the same thing.

Idea: remove the stateless first line for calculating z, call this stateless_aux(z)

splitmix64() == stateless_aux( splitmix64_x += splitmix_step )
stateless(x,i) == stateless_aux( x + (i + 1) * splitmix_step )  

Now: stateless(x, 0) == splitmix64() with seed x

Code changes are tiny.
Instead of stateless(x + i), now is stateless(x, i)

lemire commented 6 years ago

Do you want to propose a pull request?

achan001 commented 6 years ago

Sorry, I don't know what a pull request is.
If it mean add this minor changes to the code. Yes.

A stateless version should match the original (except the stateless part)

It is also nice to have the same aux function to handle both version.
Less code maintainence, say, new improved constants, or whatever.

lemire commented 6 years ago

@achan001 Yes, I mean... do you want to propose actual code?... you can share it with me any way you'd like.

achan001 commented 6 years ago

This should make both version matches: stateless(x, 0) == splitmix() w/ seed x

static inline uint64_t splitmix64_aux(uint64_t z) {
  z = (z ^ (z >> 30)) * UINT64_C(0xBF58476D1CE4E5B9);
  z = (z ^ (z >> 27)) * UINT64_C(0x94D049BB133111EB);
  return z ^ (z >> 31);
}

#define SP_STEP         UINT64_C(0x9E3779B97F4A7C15)
uint64_t splitmix64_x;  // splitmix64 internal state

static inline uint64_t splitmix64() { 
  return splitmix64_aux( splitmix64_x += SP_STEP ); 
}

static inline uint64_t splitmix_stateless(uint64_t x, uint64_t i) {
  return splitmix64_aux( x + (i + 1) * SP_STEP ); 
}
lemire commented 6 years ago

I understand the benefit of splitmix64_aux as you avoid code duplication. But I don't understand why you add a extra parameter to splitmix_stateless. This implies a refactoring of all of the rest of the code, and the new parameter will always be zero as far as I can tell. So where is the benefit? (Please note that this repository is not meant to be used as a library. It is a testing ground so one can benchmark random number generators and find out how well they do.)

achan001 commented 6 years ago

My first post stated the changes is not needed for testing PRNG.
But nice to have stateless version = stateful version of same seed

Variable i will not always be zero, unless sizeof(state) == 64 bits.
If original code was this, just replace (seed + i) to (seed, i):

int n = sizeof(state) / sizeof(state[0]);
for(int i=0; i < n; i++) 
    state[i] = splitmix_stateless(seed + i);

Again, the changes is not needed, but nice to have. If you decided no, at least put a comment for seed conversion, like this

// splitmix64_stateless(x) == splitmix64() with seed = (x - 1) * 0x9e3779b97f4a7c15
// splitmix64() w/ seed x == splitmix64_stateless(x * 0xf1de83e19937733d + 1)
lemire commented 6 years ago

Done as per the last commit.

lemire commented 6 years ago

Ok. I removed the comment.