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

Global rng #106

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Global rng #106

wants to merge 6 commits into from

Conversation

amichuda
Copy link
Collaborator

resolves #104

  • Adds ability to add a numpy.random.Generator as argument to seed
  • Changes function description and adds type hints in line with above
  • Adds test to check that adding integer seed and generator leads to same answers

@amichuda amichuda added the enhancement New feature or request label Jun 24, 2024
@amichuda amichuda requested a review from s3alfisc June 24, 2024 19:17
@amichuda
Copy link
Collaborator Author

Rather than adding a new argument, I just went ahead and allowed seed to take a random generator as an input. I think this is better than adding a new argument as it cuts down on the need for extra logic or exception handling if both seed and a hypothetical rng argument would be both specified. Let me know if that works

@amichuda amichuda mentioned this pull request Jun 26, 2024
@s3alfisc
Copy link
Member

Looks good and makes a lot of sense!

@amichuda
Copy link
Collaborator Author

amichuda commented Jun 26, 2024

@s3alfisc Great, thank you! Is there a way to re-run the CI checks now that the github action is updated?

EDIT: nvm, figured it out!

@amichuda
Copy link
Collaborator Author

So it looks like the tests for seeds are failing, but they worked yesterday, I'm going to make the tests a bit more robust and try multiple seeds.

@amichuda
Copy link
Collaborator Author

@s3alfisc

So as it stands, this part of the test_results_from_same_seed fails:

# random seed outside of function 2x -> same results
np.random.seed(123)
a2 = wildboottest(model, param = "X1", cluster = x, B= 999)
np.random.seed(123)
b2 = wildboottest(model, param = "X1", cluster = x, B= 999)
pd.testing.assert_frame_equal(a2,b2)

Right now, if you don't add a seed explicitly, wildboottest creates a default_rng with seed=None so it's not reproducible and is also not changed if the seed is changed globally.

Is setting the random seed globally like this a big use-case for py-econometrics? If so, I can work on setting it inside wildboottest, but note that numpy does not recommend this behavior:

https://numpy.org/doc/stable/reference/random/generated/numpy.random.seed.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to control sampling via an rng function argument
2 participants