daqana / dqrng

Fast Pseudo Random Number Generators for R
https://daqana.github.io/dqrng/
Other
42 stars 8 forks source link

Package loading advances R's default seed #43

Closed LTLA closed 3 years ago

LTLA commented 3 years ago

I didn't realize this before, but we can see this odd behavior:

set.seed(100)
print(.Random.seed[1:10])
##  [1]       10403         624 -1991893120 -1673479807   583080270 -1137165961
##  [7]  -848818356  -731440163  1859684602  1263821363
library(dqrng)
print(.Random.seed[1:10])
##  [1]       10403           6   592439882 -1454903371 -1649372401 -1969344490
##  [7] -1989559616  -612148093    68905121  2042642064

This is not ideal as functions using dqrng can return different results depending on whether dqrng was already loaded. You can imagine that this would be especially problematic when dqrng is a pretty deep dependency, as it is for my packages.

The culprit is the init() call in src/dqrng.cpp, which is run during package load. There are a bunch of solutions, but the one that seems best is:

  1. Set dqrng::rng64_t rng = nullptr; to avoid the package loading side-effect.
  2. Move the rng = init() call into dqset_seed(), where seed=NULL causes dqset_seed to use R's seed.
  3. Call dqset.seed(seed=NULL) in .onLoad(), making sure to save .Random.seed before the call and restore it on.exit(). This avoids advancing the seed while still initializing dqrng's seed to something sensible.
Session information ``` R Under development (unstable) (2021-01-24 r79876) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 18.04.5 LTS Matrix products: default BLAS: /home/luna/Software/R/trunk/lib/libRblas.so LAPACK: /home/luna/Software/R/trunk/lib/libRlapack.so locale: [1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C [3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8 [5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8 [7] LC_PAPER=en_US.UTF-8 LC_NAME=C [9] LC_ADDRESS=C LC_TELEPHONE=C [11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] dqrng_0.2.1.1 loaded via a namespace (and not attached): [1] compiler_4.1.0 Rcpp_1.0.6 ```
LTLA commented 3 years ago

Nudge. @rstub what do you think?

rstub commented 3 years ago

Thanks for the heads up. That sounds like a reasonable suggestion. I have started work on this in #44.

rstub commented 3 years ago

@LTLA Can you give #44 a try? I am also switching from Travis/Appveyor to Github Actions, but you can ignore those changes.

rstub commented 3 years ago

BTW, this is of course a breaking change w.r.t. current behavior. I think it is the more sensible default, but would also be interested in feedback from other package authors using dqrng: @irkaal @jlmelville @Mthrun @joemsong

jlmelville commented 3 years ago

No concerns for me.

alvindev0 commented 3 years ago

BTW, this is of course a breaking change w.r.t. current behavior. I think it is the more sensible default, but would also be interested in feedback from other package authors using dqrng: @irkaal @jlmelville @Mthrun @joemsong

Looks good to me.

joemsong commented 3 years ago

Not a problem for our packages.

Thank you.

Joe

On Sat, Apr 10, 2021 at 3:46 AM Ralf Stubner @.***> wrote:

BTW, this is of course a breaking change w.r.t. current behavior. I think it is the more sensible default, but would also be interested in feedback from other package authors using dqrng: @ircaal @jlmelville https://github.com/jlmelville @Mthrun https://github.com/Mthrun @joemsong https://github.com/joemsong

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daqana/dqrng/issues/43#issuecomment-817109383, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG3D6DRN5PNXSFVGB7K6SZTTIANATANCNFSM4YWU4P7A .

LTLA commented 3 years ago

Also looks good to me. Passes both the MRE above and my more complicated actual case.

FWIW the current seed-shifting behavior can be recovered by just tapping R's stream a few more times:

set.seed(100)
library(dqrng)
runif(6)
print(.Random.seed[1:10])
##  [1]       10403           6   592439882 -1454903371 -1649372401 -1969344490
##  [7] -1989559616  -612148093    68905121  2042642064

for anyone who was unfortunate enough to set the seed before loading the package.

LTLA commented 3 years ago

Following up on this. Are we good to go to CRAN? The next Bioconductor release is on the horizon and it would be nice to have this available before then.

Mthrun commented 3 years ago

sorry, this thread somehow got stuck in my spam folder. Please go forward with you package update. If something breaks on our side, we will fix it asap.

Best Regards Michael

On 1. May 2021, at 11:16, Ralf Stubner @.***> wrote:

Closed #43 https://github.com/daqana/dqrng/issues/43 via #44 https://github.com/daqana/dqrng/pull/44.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daqana/dqrng/issues/43#event-4673318544, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSZODETWYEYGQTWJPPM4TLTLPBFRANCNFSM4YWU4P7A.

rstub commented 3 years ago

@Mthrun No problem. @LTLA Thanks for the heads up. I am working on this now ...