rietho / IPO

A Tool for automated Optimization of XCMS Parameters
http://bioconductor.org/packages/IPO/
Other
34 stars 20 forks source link

nSlaves deprecated, update to BiocParallel #44

Closed jorainer closed 7 years ago

jorainer commented 7 years ago

Hi, just wanted to inform you that with the recent changes in xcms the nSlaves argument is deprecated (actually defunct) as we changed from the homebrew parallel processing to parallel processing using BiocParallel. Eventually you might consider to update IPO too.

Also, there might be some changes in xcms that might speed up the processing, i.e. by using MSnExp objects from the MSnbase package to hold the raw data from a full experiment it will no longer be required to read the data from the raw files in each iteration.

rietho commented 7 years ago

Thank you very much for the information. I'll look into that.

spookysounds commented 7 years ago

Hello, I've also had this issue. Is there a work around for this issue?

rietho commented 7 years ago

As long as you are using the Bioconductor release versions, there should not be an issue.

jorainer commented 7 years ago

yes and no - you will see warnings coming from the xcms methods if they are invoked with the nSlaves parameter. You can still use parallel processing in xcms e.g. by

library(BiocParallel)
register(MulticoreParam(4))

All methods in xcms will then use these globally defined settings.

Gscorreia89 commented 7 years ago

In my experience this definitely caused a problem (Linux operating system), as the number of nSlaves is deprecated error is thrown, the optimization seems to continue (not sure though...?!?) for a few more moments, but as soon as a processing finishes everything stops. In order to make IPO work with multicore I had to do a quick fix and modify the source code to remove all references to nSlaves passed to XCMS in the parameter object and function calls.

jorainer commented 7 years ago

Passing the nSlaves parameter to xcms functions does not throw an error, but just a warning - that's also why the processing continues. To set the parallel processing just do:

library(BiocParallel)
register(MulticoreParam(3))

This sets the parallel processing globally to use 3 cores using the multicore package. Alternatively one could use SnowParam or SerialParam (to completely disable parallel processing).

Gscorreia89 commented 7 years ago

Yep. But for some reason IPO cannot assemble the results when the warning is returned though - If I wait long enough for the processing to be finished, XCMS does the job normally, but the IPO object is never assembled and the figures for the DoE parameter search are never saved.

rietho commented 7 years ago

It's also not clear for me, why your IPO runs fail to run correctly. This issue was not on my priority lists until now, but I'll definitely fix the nSlaves-parameter during the next weeks (sorry for the long wait).

rietho commented 7 years ago

@jotsetung I'm working on updating IPO to call xcms with BPPARAM instead of nSlaves. Therefore I'll introduce a new parameter BPPARAM_xcmsSet to optimizeXcmsSet (the params argument is not designed to hold an S4-class object). Nevertheless I'll keep the nSlaves parameter as deprecated. On the devel-branch of xcms you are using NULL as default parameter for nSlaves, but on the current Bioconductor devel you are using 0 as default value. So I'm wondering, if 0 will stay the default for the Bioconductor 3.5 release?

jorainer commented 7 years ago

actually, you would not really use the BPPARAM, the default for this parameter in xcms is bpparam() which just returns the globally defined parallel processing. The global settings could be set using the register method from BiocParallel:

register(MulticoreParam(4))

I think it does not really matter what value you use for nSlaves, I think 0 should be OK.

rietho commented 7 years ago

Doesn't BPPARAM exist to give the user the possibility to use BiocParallel without using global settings? I also chose bpparam() as default, so that global settings would still work.

jorainer commented 7 years ago

Yes, it allows to change parallel processing settings for individual tasks - although I usually use the same settings across an analysis

rietho commented 7 years ago

Ok, great. I'll keep BPPARAM to let the user choose. I won't call it BPPARAM_xcmsSet, to leave the option of using BiocParallel by IPO itself.

rietho commented 7 years ago

see d2ae262f2208eff62ab2f4a3ca5d6e0ef66c6976

rietho commented 7 years ago

@Gscorreia89 Does this reslove your issues?

Gscorreia89 commented 7 years ago

@rietho I fixed all my problems by applying a similar patch, so I am assuming it will work now. When I have some time I will test your update and let you know if all is sorted in the "official" patch.