-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fully initialize state when setting the seed (fixes #3) #7
Conversation
So two things. First, are we really sure that the Second, what do mean by "extend the tests" (ie add more?) and by "unsure how it works"? We use the |
See for example a use of seeding with the current CRAN version of the package, ie without your patch: edd@brad:~$ Rscript -e 'library(RcppZiggurat); zsetseed(456); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.376230 -0.730005
[1] 0.1897669 -0.7272849
edd@brad:~$ Rscript -e 'library(RcppZiggurat); zsetseed(456); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.376230 -0.730005
[1] 0.1897669 -0.7272849
edd@brad:~$ We can set a seed perfectly well, and reproduce the results. /cc #3 |
From my point of view
The first call reproduces your example. The second call shows that using the same seed '789' yields a different result if it is used as first seed. The third example shows that using the same seed multiple times within one R session gives different results (that was the original issue from SO). This is different from base functionality:
With the patch applied, I get reproducible random numbers that are independent of the RNG's history:
I will extend the PR w.r.t |
* Additional test that checks that repeatedly setting the same seed in one session yields the same result. * Adjust the expected output from `summary(zrnorm(1e6))`, since now the result does not depend on the previous calls to `zsetseed()`.
Let me think about this:
My gut reaction is that this is simply how Ziggurat works / is designed.
Dittto But your third example is good. I think I may have tried to be too-clever-by-half and have the assignment your PR brings already in the class so that it happens at instantiation. What I overlooked was the reset-during-session functionality your third examples shows. I am warming up to this. But still early-ish morning here :) |
Take your time. :-) Just one more detail. I do not think that this is specific to the Ziggurat method, since ZigguratLZLLV behaves the way I am expecting (and also sets
|
I would also like to extend the tests, but I am unsure how those work.