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

Merged
merged 2 commits into from Oct 16, 2017

Conversation

Projects
None yet
2 participants
@rstub
Contributor

rstub commented Oct 9, 2017

I would also like to extend the tests, but I am unsure how those work.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 9, 2017

So two things.

First, are we really sure that the jsr = 123456789; assignment is needed? After all, it is set in all in the derived classes we actually instantiate --- inst/include/*. I don't yet see a use case where you need this (and I think #3 and the SO question are wrong).

Second, what do mean by "extend the tests" (ie add more?) and by "unsure how it works"? We use the tests/ directory with reference output as created by R CMD check. See eg "Writing R Extensions".

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 9, 2017

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

@rstub

This comment has been minimized.

Contributor

rstub commented Oct 9, 2017

From my point of view zsetseed() should set the RNG to a definite state irrespective of the history. The current implementation in Ziggurat.h uses the current state of jsr. This makes the RNG history dependent. Simple examples using the current CRAN version:

ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(456); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.376230 -0.730005
[1]  0.1897669 -0.7272849
ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(789); print(zrnorm(2))'
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(789); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.1285849 -0.1585969
[1] 0.03028204 1.36236769

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:

ralf@pc-ralf:~/devel$ Rscript -e 'set.seed(789); print(rnorm(2)); set.seed(789); print(rnorm(2))'
[1]  0.5240967 -2.2607679
[1]  0.5240967 -2.2607679

With the patch applied, I get reproducible random numbers that are independent of the RNG's history:

 ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(456); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.376230 -0.730005
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(456); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.376230 -0.730005
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(789); print(zrnorm(2))'
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel$ Rscript -e 'library(RcppZiggurat); zsetseed(789); print(zrnorm(2)); zsetseed(789); print(zrnorm(2))'
[1] -1.1285849 -0.1585969
[1] -1.1285849 -0.1585969

I will extend the PR w.r.t zigguratTest.R.

Ralf Stubner
Adjust tests for fully initialized state
* 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()`.
@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 9, 2017

Let me think about this:

The second call shows that using the same seed '789' yields a different result if it is used as first seed.

My gut reaction is that this is simply how Ziggurat works / is designed.

This is different from base functionality

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 :)

@rstub

This comment has been minimized.

Contributor

rstub commented Oct 9, 2017

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 jsr in setSeed()):

ralf@pc-ralf:~/devel/rcppziggurat/inst/include$ Rscript -e 'library(RcppZiggurat); zsetseedLZLLV(456); print(zrnormLZLLV(2)); zsetseedLZLLV(789); print(zrnormLZLLV(2))'
[1] -1.376230 -0.730005
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel/rcppziggurat/inst/include$ Rscript -e 'library(RcppZiggurat); zsetseedLZLLV(456); print(zrnormLZLLV(2)); zsetseedLZLLV(789); print(zrnormLZLLV(2))'
[1] -1.376230 -0.730005
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel/rcppziggurat/inst/include$ Rscript -e 'library(RcppZiggurat); zsetseedLZLLV(789); print(zrnormLZLLV(2))'
[1] -1.1285849 -0.1585969
ralf@pc-ralf:~/devel/rcppziggurat/inst/include$ Rscript -e 'library(RcppZiggurat); zsetseedLZLLV(789); print(zrnormLZLLV(2)); zsetseedLZLLV(789); print(zrnormLZLLV(2))'
[1] -1.1285849 -0.1585969
[1] -1.1285849 -0.1585969

@eddelbuettel eddelbuettel merged commit bfbfd62 into eddelbuettel:master Oct 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rstub rstub deleted the rstub:issue3 branch Oct 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment