static random-number generator 'defeated' by seed in constructor

[mmurrian]

Every construction of an EigenMultivariateNormal instance re-seeds the static random-number generator. This will cause all instances to reset their RNG sequence back to the same output whenever a new EigenMultivariateNormal is created. If samples() is only called once per instance, the output is always the same!

I recommend providing a static setSeed(...) function that the user can explicitly call and not setting the seed in the constructor.

[mmurrian]

See pull request #2

[beniz]

My understanding is that the twister seeds to a fixed seed by default, see https://www.cplusplus.com/reference/random/mt19937/ so what you need to do here is to seed differently yourself and call the constructor.

[mmurrian]

// Construct random-number generator #1
EigenMultivariateNormal mvnrnd1(mean, covar);

while(1) {
  // Construct another random-number generator
  EigenMultivariateNormal mvnrnd2(mean, covar);

  const auto sample1 = mvnrnd1.samples(1);
  const auto sample2 = mvnrnd2.samples(1);
}

In this example, sample1 will always generate the same value.

The problem isn't that mvnrnd1 and mvnrnd2 generate the same sequence (they don't). The problem is that mvnrnd1 gets stuck at the same value because mvnrnd2 is being constructed. Why should constructing one random-number generator break another one?

[beniz]

Have you tried have the twister non static on line https://github.com/beniz/eigenmvn/blob/master/eigenmvn.h#L51?

[mmurrian]

I fixed the bug per pull request #2. Making the twister non-static is another solution.

Given the purpose of your code, I think having a static twister is the better choice. If multiple instances of EigenMultivariateNormal are drawing from the same twister in, say, multiple, unsynchronized threads... you can potentially get true randomness out it.

But there are valid reasons to make it non-static (if you actually want a deterministic, pseudorandom output).

Either way, the current behavior is buggy.