-
Notifications
You must be signed in to change notification settings - Fork 388
[API change] Making the seeding more intuitive. #615
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
Conversation
|
I realised that my understanding of libraries producing random numbers is really bad haha, let's close that for now and I'll come up with a better proposal later on. |
|
Now I am curious, what it is that you discovered, that made you write this. |
|
Basically, I didn't know that there was a way to generate random numbers without touching the global seed. RandomStates should be encouraged because touching the global random seed isn't thread safe. Furthermore, you can't know if some other libraries will mess up the global random state without your knowledge. See #573. In the end, to allow isolation, we should use random numbers generators, like it is currently done in the sacred API. One thing I'd like to change though, is the automatic seeding. Touching a global variable without explicit user approval can lead to some unexpected results if the user skip this part of the docs. |
After a lot of thinking, here is my proposal for the new seeding setup.
Fixes #432
Fixes #586
Fixes part of #610 #193
My take here is:
We add too little value compared to a manual seeding done by the user and passed through the config. In the end, it requires a lot of documentation and engineering for a very simple result: Seeding when the experiment starts and get some seeds here and there. This can be done in a couple of lines by the user, and will be much more explicit and clear. We can encourage users to put their seed in the config so that it's saved and can be changed from the command line.
This is only a couples of LOC for the user, but a lot less LOC for sacred. This should prepare the ground to make something simpler for the config as mentioned in #610.
Of course this is a big breaking change. Because of this, we'll use the strategy of the global flag to avoid users shooting themselves in the foot.
I'm not sure the PR actually works if the flag is set to False. The doc isn't updated. This PR serves as a base for discussion about the behavior that we want.
We provide two things: