xfim / ggmcmc

Graphical tools for analyzing Markov Chain Monte Carlo simulations from Bayesian inference
111 stars 31 forks source link

Suggestion: Make parallel = FALSE by default #24

Closed dmenne closed 11 years ago

dmenne commented 11 years ago

Since setting up parallel is quite a bit of additional work (especially on Windows), I suggest to make parallel=FALSE by default to avoid many warnings (or suppress the warnings)

xfim commented 11 years ago

I am not sure, dmenne, and I am happy to discuss it. Thank you for raising the issue and insisting on it.

Let me explain my first approach: I thought that it would be better to design ggmcmc with parallelization in mind, and so making the defaults reasonable for any modern computer doing MCMC. I think that it is reasonable to expect that MCMC analysts use at least two CPUs. So taking the most of modern computers by running parallel chains in the different cores I think it is quite reasonable. So I thought, ¿why not creating a package ready for parallel computing from the beginning?

But you are right that what takes two lines of code and an additional package in GNU/Linux and MAC can be more problematic in Windows. But even in this case, the situation would be a clear win for GNU/Linux and MAC and some error messages in Windows, by default. This would still justify the default.

However, what is really discouraging me is the fact that all tests and benchmarks that I have done so far comparing the performance of ggmcmc with and without the parallel backend do not show any significant gain favoring the parallelized version. What takes most of the time in ggmcmc is the actual plotting, not the operations done by ddply().

To sum up: we have a situation in which the default setting, aimed for speed, does not make many or any difference at all, where for Windows users produces warnings and where it requires two lines of additional code for GNU/Linux and Mac users.

So following your suggestion, I think that we seriously have to think to make parallel=FALSE the default, but I'm not entirely sure. Again, thank you for your input.

Do you have any other arguments? Any other thoughts?

dmenne commented 11 years ago

Your observation about parallelization is perfectly right, and it is typical for the disappointment frequently reported on Stack Overflow. I use parallelization under Windows only to run rstan where we have full efficiency for multiple chains, but setting it up is not that easy. For those who want to try, see below. ggplot is much slower than lattice, and MCMC plots are a bit biggish. Anyway, in most stan runs MCMC is limiting, so I would take the easy way out and avoid parallelization build in. As an alternative, you could define an option; or, even more elegant, check if parallelization has been set up and skip (NO warnings, please) it if not.

mr1.stan =  stan(file = str_c(Model,'.stan'), chains=0, init = init,data=data)

stanFit = function(i){
  library(rstan) # Must be repeated
  stan(fit = mr1.stan, seed = seed, chains = 1, iter=6000,
     chain_id = i, refresh = -1,data=data,init=init)
} 

logfile = str_c(Model,".log")
dummy = suppressWarnings(file.remove(logfile))
nCores = 4 # Number of cpu cores
cl <- makeCluster(nCores,outfile=logfile)
clusterExport(cl, c("stanFit","mr1.stan","seed","init","data","stan"))
system.time(processed <- parLapply(cl,1:chains, stanFit))
stopCluster(cl)

mr.stan = sflist2stanfit(processed)
dmenne commented 11 years ago

Now many of the other warnings are gone, it would be really nice to get rid of these. I know, I am a bit picky if it comes to warnings, but when there are too many irrelevant ones, it's easy to miss important things that come up as one-liners

1: In setup_parallel() : No parallel backend registered 2: In setup_parallel() : No parallel backend registered 3: In setup_parallel() : No parallel backend registered 4: In setup_parallel() : No parallel backend registered 5: In setup_parallel() : No parallel backend registered 6: In setup_parallel() : No parallel backend registered

xfim commented 11 years ago

Ok, I have left parallel=TRUE and will try to better document the pros and cons of using it, but at the same time I have added a call to suppressWarnings() before any ddply call that uses .parallel. I hope that this is the best of both worlds: get rid of warning messages related to the parallel backed and also bank on the default use of parallelization.

Solved in commit #a2306c6.

dmenne commented 11 years ago

Sounds good. An example how to initialize the parallel front end that works under Windows and Linux would be nice. I work a lot with parallel, but I do not consider it easy.

xfim commented 11 years ago

Ok, I have been doing extensive benchmarks and the situation has led me to rethink again the default of parallel=TRUE. Even with 100,000 iterations, 3 chains and 3 parameters the sequential version beats the parallelized. And making it work in Windows to provide a decent example is giving me lot of trouble. So I think that it is time to accept that pushing for parallelization when it really does not add much is counterproductive. Therefore, I will make it FALSE by default.

dmenne commented 11 years ago

Thanks. Being disappointed when doing parallelization is well known to me.I would even go further and eliminate it totally, but as far it does not come in the way, that's ok.

xfim commented 11 years ago

Perfect. Given that the default is now not to use parallel, I will revert the suppressWarnings() calls before any ddply() call and make the code cleaner.