Merck / simtrial

Clinical trial simulation for time-to-event endpoints
https://merck.github.io/simtrial/
GNU General Public License v3.0
16 stars 8 forks source link

Fixing sim_fixed_n() parallel results return #252

Closed cmansch closed 3 months ago

cmansch commented 3 months ago

This update addresses two open issues: https://github.com/Merck/simtrial/issues/250 and https://github.com/Merck/simtrial/issues/251.

In order to return the results from each %dofuture% loop, the last object must return some value. The setDF() call previously used resulted in the results not being passed back from the loop when run with a parallel backend. This error was hidden in the results as the loop used the .errorhandling = "pass" instead of "stop".

The parallel vignette was also updated to indicate that sim_gs_n() supports the same backend implementation as sim_fixed_n().

jdblischak commented 3 months ago

I get an error when running it locally. Am I the only one?

git2r::commits(n = 1)
## [[1]]
## [443a59c] 2024-05-28: Updating the parallel vignette to indicate that `sim_gs_n()` also supports the `future` backends.
devtools::load_all()
library("future")
plan("multisession", workers = 2)
sim_fixed_n(n_sim = 10)
## Using 2 cores with backend multisession
## Error in doAnalysis(d, rho_gamma, n_stratum) :
##   could not find function "doAnalysis"
sessionInfo()
## R version 4.3.3 (2024-02-29 ucrt)
## Platform: x86_64-w64-mingw32/x64 (64-bit)
## Running under: Windows 11 x64 (build 22631)
## 
## Matrix products: default
## 
## 
## locale:
##   [1] LC_COLLATE=English_United States.utf8  LC_CTYPE=English_United States.utf8   
## [3] LC_MONETARY=English_United States.utf8 LC_NUMERIC=C                          
## [5] LC_TIME=English_United States.utf8    
## 
## time zone: America/New_York
## tzcode source: internal
## 
## attached base packages:
##   [1] stats     graphics  grDevices utils     datasets  methods   base     
## 
## other attached packages:
##   [1] future_1.33.2    simtrial_0.4.1.3 testthat_3.2.1.1
## 
## loaded via a namespace (and not attached):
##   [1] lattice_0.22-6      stringi_1.8.4       listenv_0.9.1       digest_0.6.35      
## [5] magrittr_2.0.3      grid_4.3.3          iterators_1.0.14    mvtnorm_1.2-4      
## [9] pkgload_1.3.4       fastmap_1.1.1       Matrix_1.6-5        foreach_1.5.2      
## [13] rprojroot_2.0.4     pkgbuild_1.4.4      sessioninfo_1.2.2   brio_1.1.5         
## [17] survival_3.6-4      urlchecker_1.0.1    promises_1.3.0      purrr_1.0.2        
## [21] codetools_0.2-20    cli_3.6.2           shiny_1.8.1.1       rlang_1.1.3        
## [25] parallelly_1.37.1   future.apply_1.11.2 splines_4.3.3       ellipsis_0.3.2     
## [29] remotes_2.5.0       withr_3.0.0         cachem_1.0.8        devtools_2.4.5     
## [33] tools_4.3.3         parallel_4.3.3      memoise_2.0.1       doFuture_1.0.1     
## [37] httpuv_1.6.15       globals_0.16.3      vctrs_0.6.5         R6_2.5.1           
## [41] mime_0.12           lifecycle_1.0.4     git2r_0.33.0        stringr_1.5.1      
## [45] fs_1.6.4            htmlwidgets_1.6.4   usethis_2.2.3       miniUI_0.1.1.1     
## [49] desc_1.4.3          later_1.3.2         glue_1.7.0          data.table_1.15.4  
## [53] profvis_0.3.8       Rcpp_1.0.12         rstudioapi_0.16.0   xtable_1.8-4       
## [57] htmltools_0.5.8.1   compiler_4.3.3
jdblischak commented 3 months ago

Hmmm...the problem appears to be with devtools::load_all()

git2r::commits(n = 1)
## [[1]]
## [443a59c] 2024-05-28: Updating the parallel vignette to indicate that `sim_gs_n()` also supports the `future` backends.
library("simtrial")
library("future")
plan("multisession", workers = 2)
sim_fixed_n(n_sim = 10)
# works
jdblischak commented 3 months ago

I think I figured it out. In short, you can't use devtools::load_all() on Windows to test parallel code that uses "multisession". You can only use devtools::load_all() on unix-alike with "multiprocess" or "multicore. See this Issue for all the details https://github.com/HenrikBengtsson/future/issues/206

jdblischak commented 3 months ago

One minor nit: since this fixes a known bug that is affecting end users, it would be useful to bump the version number in DESCRIPTION to 0.4.1.4 so that users can confirm they have the fix installed

cmansch commented 3 months ago

One minor nit: since this fixes a known bug that is affecting end users, it would be use to bump the version number in DESCRIPTION to 0.4.1.4 so that users can confirm they have the fix installed

Added a commit to address the version bump!