bedapub / designit

Blocking and randomization for experimental design
https://bedapub.github.io/designit/
Other
7 stars 1 forks source link

things to fix before resubmitting to CRAN #44

Closed idavydov closed 5 months ago

idavydov commented 6 months ago

While working on this I removed R/randomization.R; we have several unexported functions which were responsible for most of the warnings. We can later recover the file if needed.

idavydov commented 6 months ago

Please do not set a seed to a specific number within a function

Here's the relevant code:

  # based on https://stat.ethz.ch/pipermail/r-help/2007-September/141717.html
  if (!exists(".Random.seed")) stats::runif(1)
  trace$seed <- list(.Random.seed)

I personally do not think this qualifies as setting a seed; however I agree that this is not an idiomatic code.

@klmr @banfai @ingitwetrust what's your suggestions?

the path of least resistance is just removing this.

klmr commented 6 months ago

Without prior knowledge of the structure it isn’t obvious to me what the purpose of trace$seed is.

If the purpose is reproducibility, it should be sufficient to replace the code in question with

trace$seed <- list(get0(".Random.seed", .GlobalEnv))

This will assign NULL if the seed isn’t yet set. However, this now requires special handling when using the stored seed, since NULL is an invalid random seed.

And note that it’s not sufficient to store the random seed to get full reproducibility, we also need to store the RNG kind and version.

klmr commented 6 months ago

Oops, the previous comment is nonsense: storing NULL is obviously not sufficient to get reproducibility. We do actually need to seed the RNG under these circumstances; but we should use the proper seeding function for that instead of using the side-effect from runif(1).

There doesn’t seem to be an idiomatic way of seeding the RNG with a good random number using set.seed(). However, the following should do the trick, and I would call it idiomatic based on the examples from the documentation:

if (!exists(".Random.seed")) {
  # Explicitly set the default RNG in order to seed it with a random value that we can store.
  do.call(RNGkind, as.list(RNGkind()))
}

(Actually the documentation is incorrect because it implies that RNGkind() should also work and select the default RNG, but that is not the case.)

idavydov commented 6 months ago

thanks, @klmr. I updated the code to save RNG kind and I do not use side-effects of runif(). I also extensively commented this fragment and added tests.

could you please review?

idavydov commented 6 months ago

Facilitate experiment design...

idavydov commented 6 months ago

hi @klmr, I think it's ready for review now.

@ingitwetrust regarding design of experiment, I forgot but it's in the title. so should be quite findable.