Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disable nested parallel processing for chromatograms #602

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Conversation

jorainer
Copy link
Collaborator

This fix addresses a comment in issue
sneumann/xcms#627 (comment).

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.

@jorainer jorainer requested a review from lgatto April 30, 2024 08:39
@lgatto
Copy link
Owner

lgatto commented Apr 30, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Documentation file issues should be fixed too.

@lgatto lgatto merged commit 4d23ce3 into master Apr 30, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants