Don't default seed to 1?

droodman commented 3 years ago

https://github.com/s3alfisc/wildboottestjlr/blob/2c9e8686517d60edd00ad882daac839b1c0af3e9/R/boottest_lm.R#L167

I'm not that familiar with the issues mechanism, so this is an experiment for me. Is this a good way to discuss the code?

At least in Stata, I think it would be bad form for a program to set the RNG seed without the user requesting it. The situation here is unusual since the RNGs of Julia and R are running separately, But in general, as user might want to set the seed once at the top of a program and then call lots of procedures that generate random numbers and push the RNG algorithm forward, never resetting it. And probably 1 would be a considered a bad starting point.

In Julia, if you want to instantiate an RNG without setting the seed, you just drop the argument. You can do MersenneTwister() instead of MersenneTwister(12313). Maybe the R interface could just let the user pass a string such as "StableRNG(2938472394)" or "MersenneTwister()" or "Xoroshori256++(4564)". Probably it would be fine to limit it to the three RNGs just mentioned. StableRNG() is in the StableRNGs package. I think the other two will be built into Base in Julia 1.7, but we'll see.

s3alfisc commented 3 years ago

I definitely think that it is a good idea to discuss code over here - it guarantees that our discussion will not be lost in our emails. Also, it is quite easy to link issues to commits, which should help us keep track of how our discussion affects the code.

Using 1 as a seed is a relic from the implementation in fwildclusterboot. I understand why setting an internal set might not be good practice and I will change it here / have changed it in fwildclusterboot. I suppose that the main idea was to guarantee reproducibility when the user does not set a "global" seed outside of the boottest() function.

In general, I think that it would be nice if fwildclusterboot::boottest() and wildboottestjlr::boottest() shared many of their most important function arguments, so that users who have written code based on one package could easily switch back and forth between both packages.

I have now switched to using the dqrng R package to generate the bootstrap weights in fwildclusterboot (which improves performance by a lot), which requires the user to use a specific seed function, dqrng::dqset.seed(). To make it clear to the user that he / she cannot use the more common set.seed() function to set a seed, I might deprecate the seed function argument and introduce a new argument, "rng".

So for now I will

get rid of the internal seeding within boottest()
set an option to feed the users "seed" argument into a string, hence boottest(..., rng= 2) would be evaluated in Julia as "StableRNG(2)". Once Julia 1.7 is available, we can include an option to choose the the random number generator (but we might have to make the package dependent on Julia 1.7 in this case?).

droodman commented 3 years ago

Sounds good. Hopefully it would still work with earlier versions and only generate an error if the user explicitly chose Xoroshiro.

s3alfisc commented 3 years ago

   if(is.null(rng)){
      rng <- "Random.MersenneTwister()"
    } else{
      rng <- paste0("Random.MersenneTwister(", rng, ")")
    }

is now passed to wildboottest() via juliaLet().

s3alfisc commented 3 years ago

Random seeding implemented via MersenneTwister.

s3alfisc / wildboottestjlr

Don't default seed to 1? #1