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

pass RNG instead of seeds #281

Merged
merged 1 commit into from
May 31, 2023
Merged

pass RNG instead of seeds #281

merged 1 commit into from
May 31, 2023

Conversation

costachris
Copy link
Member

@costachris costachris commented May 4, 2023

Closes #260

Pass RNG objects instead of seeds to make randomness more traceable.

@odunbar
Copy link
Collaborator

odunbar commented May 19, 2023

@costachris Could you rebase with main, before I review, hopefully then all tests will pass! Cheers

@costachris
Copy link
Member Author

costachris commented May 26, 2023

@odunbar The EKP test is failing because we're doing two sequential calls to construct_initial_ensemble, passing the same rng object without reseeding, and checking that they're equal. This test made sense when we were passing the seed to construct_initial_ensemble, but that's no longer the case. The simple solution is to reseed the rng object right before the second call to construct_initial_ensemble (outside of the function) in the test, but just want to double-check that's the behavior we want?

Copy link
Collaborator

@odunbar odunbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, Thank you! Regarding your comment. I think we should just remove that test.

As you say, the reproducibility should not occur unless the user does something external now - by explicitly copying/reseeding the rng, or the GLOBAL_RNG outside the function. This is then something already tested in Random/Distributions etc. so not our concern any more.

Once this is done, please merge!

@costachris
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented May 31, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 91db990 into main May 31, 2023
11 checks passed
@bors bors bot deleted the pass_rng branch May 31, 2023 17:23
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.

Should be passing an RNG here, not just a seed
2 participants