lgatto / MSnbase

Base Classes and Functions for Mass Spectrometry and Proteomics
http://lgatto.github.io/MSnbase/
123 stars 50 forks source link

fix: disable nested parallel processing for chromatograms #602

Closed jorainer closed 2 months ago

jorainer commented 2 months ago

This fix addresses a comment in issue https://github.com/sneumann/xcms/issues/627#issuecomment-2080333123.

chromatogram() is extracting chromatograms in parallel for each sample/file. The internal function uses spectra() to load the data in each parallel process. The default value for BPPARAM of spectra() is however BPPARAM = bpparam(), thus, an eventual nested (and unnecessary) parallel processing setup is created. This PR forces the internal spectra() call to disable parallel processing.

lgatto commented 2 months ago

Running checks locally, and I can confirm the issues above:

> ### Name: chromatogram,MSnExp-method
> ### Title: Extract chromatogram object(s)
> ### Aliases: chromatogram,MSnExp-method chromatogram
> 
> ### ** Examples
> 
> ## Read a test data file.
> library(BiocParallel)
> register(SerialParam())
> library(msdata)
> f <- c(system.file("microtofq/MM14.mzML", package = "msdata"),
+      system.file("microtofq/MM8.mzML", package = "msdata"))
> 
> ## Read the data as an MSnExp
> msd <- readMSData(f, msLevel = 1)
> 
> ## Extract the total ion chromatogram for each file:
> tic <- chromatogram(msd)
Error: BiocParallel errors
  1 remote errors, element index: 1
  1 unevaluated and other errors
  first remote error:
Error in .local(object, ...): unused argument (BPPARAM = new("SerialParam", .xData = <environment>))
jorainer commented 2 months ago

Ah, sorry, locally unit tests fail for me because of preprocessCore... the spectra,MSnExp does not have a parameter BPPARAM (is also not needed) and it thus fails. I'll add ... to spectra,MSnExp to avoid this.

jorainer commented 2 months ago

Documentation file issues should be fixed too.