Merck / simtrial

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

Example 4 of `sim_fixed_n` does not work #251

Closed LittleBeannie closed 5 months ago

LittleBeannie commented 5 months ago

Hi @cmansch , it looks the last example of sim_fixed_n does not work, which is related to parallel computation. Could you please help take a look at it?

Also looping in @jdblischak. It looks like the stylize codes update may change how the results returned. https://github.com/Merck/simtrial/blob/04b36d068e4a3b9b6a8a6c739fa9576be2a556fc/R/sim_fixed_n.R

image

jdblischak commented 5 months ago

Interesting. I think we stumbled across the same problem a few weeks in @cmansch's PR to parallelize sim_gs_n() (https://github.com/Merck/simtrial/pull/249). When he updated sim_fixed_n() to use .errorhandling = "stop", the vignettes started failing with the error object 'results' not found (https://github.com/Merck/simtrial/pull/249#issuecomment-2115963072), just like above. Thus fixing this Issue should also fix the issue to update the error handling of sim_fixed_n() (https://github.com/Merck/simtrial/issues/250).

I'm going to investigate more now. But the current situation is that the parallel executions of sim_fixed_n() in the examples and vignettes only succeed because the setting .errorhandling = "pass" does not fail when errors occur. So now the question is: did parallel execution of sim_fixed_n() work when it was first introduced in https://github.com/Merck/simtrial/pull/110? And if yes, what changed?

jdblischak commented 5 months ago

I'm not sure parallel execution of sim_fixed_n() ever worked. At least not with the latest versions of {future}, {doParallel}, etc. I checked out the merge commit of PR #110, 341f77f0a598dc6d638bd5c48746952a7db88255, and it fails with the same error.

git checkout 341f77f0a598dc6d638bd5c48746952a7db88255
git log -n 1 --oneline
## 341f77f (HEAD) Merge pull request #110 from cmansch/main
Rscript -e 'devtools::load_all(); library("future"); plan("multisession", workers=2); sim_fixed_n(n_sim = 10)'
## ℹ Loading simtrial
## Using 2 cores with backend multisession
##           message                                   call
## result.1  "unused argument (rho_gamma = rho_gamma)" expression
## result.2  "unused argument (rho_gamma = rho_gamma)" expression
## result.3  "unused argument (rho_gamma = rho_gamma)" expression
## result.4  "unused argument (rho_gamma = rho_gamma)" expression
## result.5  "unused argument (rho_gamma = rho_gamma)" expression
## result.6  "unused argument (rho_gamma = rho_gamma)" expression
## result.7  "unused argument (rho_gamma = rho_gamma)" expression
## result.8  "unused argument (rho_gamma = rho_gamma)" expression
## result.9  "unused argument (rho_gamma = rho_gamma)" expression
## result.10 "unused argument (rho_gamma = rho_gamma)" expression
Rscript -e 'library("doParallel"); library("future"); sessionInfo()'
## [1] future_1.33.2     doParallel_1.0.17 iterators_1.0.14  foreach_1.5.2
cmansch commented 5 months ago

I found that the current issue is related to how setDF(results_sim) does not return anything, which results in the results being empty for the parallel implementation. This issue will be addressed in a PR soon.

cmansch commented 5 months ago

I believe that this will also explain why changing the error handling to stop caused an issue and that we should use this .errorhandling option moving forward!