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

Remove redundant seed #180

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Remove redundant seed #180

merged 1 commit into from
Mar 29, 2021

Conversation

FloopCZ
Copy link
Contributor

@FloopCZ FloopCZ commented Mar 22, 2018

I believe those seed commands are unnecessary after the constructor with random device.
Moreover, this only causes that seeding in the same second will produce the same number.

@beniz
Copy link
Collaborator

beniz commented Mar 26, 2018

Hi, default seed in C++ for the Mersenne twister engine is is 589u, see http://www.cplusplus.com/reference/random/mersenne_twister_engine/mersenne_twister_engine/
We really want a random seed here.

If you have use case where the seed needs a sub-second init, let's review it here.

@FloopCZ
Copy link
Contributor Author

FloopCZ commented Mar 26, 2018

Hi @beniz, this is a misunderstanding. Naturally, I would not bother you with a PR initializing mt19937 with a fixed value, I will try to explain the problem. 🙂

Take a look at the affected lines:

    std::random_device rd;
    _uhgen = std::mt19937(rd());
    _uhgen.seed(static_cast<uint64_t>(time(nullptr)));  // This line is removed by this PR.

You create std::random_device (i.e., a link to /dev/urandom or a similar source of truly random numbers) and pass a random number from this device to the Mersenne twister's constructor as a seed. So there is no need to seed it again using the current time. On the contrary, if you seed it again like this:

    _uhgen.seed(static_cast<uint64_t>(time(nullptr)));

, then the seed value changes only every second, so constructing multiple Mersenne twisters at the same second actually means that they will generate the same sequence of random numbers.

@FloopCZ
Copy link
Contributor Author

FloopCZ commented Mar 28, 2021

Hi @beniz, it has been a while. 🙂
Do you think it would be possible to take another look at this PR and my last comment? I hope it explains the current bug and why it is fixed by the proposed PR.

@beniz
Copy link
Collaborator

beniz commented Mar 29, 2021

Hi @FloopCZ this may have slipped below my radar at the time. You are correct the twister is wrongly seeded twice, with the second seeding being worse than the first one.

Your last commit is breaking this PR though, what you want is to do a rebase to master, or resubmit a PR, and I'll merge it asap.

Thanks for your patience and persistence!

I believe those seed commands are unnecessary after the constructor with random device.
Moreover, this only causes that seeding in the same second will produce the same number.
@FloopCZ
Copy link
Contributor Author

FloopCZ commented Mar 29, 2021

Your last commit is breaking this PR though, what you want is to do a rebase to master, or resubmit a PR, and I'll merge it asap.

Rebased and force pushed!

@beniz beniz merged commit 0c4d1c1 into CMA-ES:master Mar 29, 2021
@FloopCZ FloopCZ deleted the redundant-seed branch March 30, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants