Skip to content
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

things to fix before resubmitting to CRAN #44

Merged
merged 18 commits into from
Mar 19, 2024
Merged

things to fix before resubmitting to CRAN #44

merged 18 commits into from
Mar 19, 2024

Conversation

idavydov
Copy link
Collaborator

@idavydov idavydov commented Mar 13, 2024

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.

  • proofread DESCRIPTION
  • If there are references describing the methods in your package, pleas add these in the description
  • examples for unexported functions
examples or export these functions. -> Warning: Examples for unexported
function
   random_score_variances() in:
      randomize.Rd
  • \dontrun{} should only be used if the example really cannot be executed
  • Instead of cat() rather use message()/warning() or if(verbose)cat(..) -> R/randomization.R
  • do not set a seed to a specific number within a function. -> R/optimize.R
  • do not modify the global environment (e.g. by using <<-) tests/testthat/test_optimize_design_simple_shuffle.R
  • add @Accio

@idavydov idavydov requested a review from klmr March 13, 2024 14:26
@idavydov idavydov marked this pull request as draft March 13, 2024 14:26
@idavydov idavydov self-assigned this Mar 13, 2024
@idavydov
Copy link
Collaborator Author

idavydov commented Mar 13, 2024

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
Copy link
Collaborator

klmr commented Mar 15, 2024

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
Copy link
Collaborator

klmr commented Mar 15, 2024

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
Copy link
Collaborator Author

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 idavydov marked this pull request as ready for review March 15, 2024 10:52
@idavydov
Copy link
Collaborator Author

Facilitate experiment design...

R/optimize.R Outdated Show resolved Hide resolved
@idavydov
Copy link
Collaborator Author

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.

Copy link
Collaborator

@klmr klmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@idavydov idavydov merged commit 545cbe1 into main Mar 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants