groupval function for old "xcmsSet" broken in xcms 3.10.0

[andzajan]

Hi, for some "historical" reasons I still have to use old "xcmsSet" object instead of new "XCMSnExp" class. So I am using xset <- as(xset, "xcmsSet") to convert new class object to the old one.

This was working perfectly fine till the latest Bioconductor 3.11 release and xcms version bump to 3.10.0. Even now current windows binary for 3.11 release still has xcms 3.9.1 package and that one is working fine.

I have tested this on M$ Windows 10 (did compile 3.10 version from source using Rtools), Ubuntu 20.04 and Travis Linux build.

With new version code below returns error:

Code example:

library(xcms)
library(BiocManager)
if(!requireNamespace("msPurityData", quietly=TRUE)){
    BiocManager::install("msPurityData")
}

mzml_files <- dir(system.file("extdata/lcms/mzML", package="msPurityData"),
        full.names=TRUE, recursive=TRUE)
rawf <- MSnbase::readMSData(mzml_files[1:2], mode="onDisk", msLevel.=1)
cwp <- xcms::CentWaveParam(snthresh=10)
xset <- xcms::findChromPeaks(rawf, param=cwp)
xset <- xcms::groupChromPeaks(xset,
        xcms::PeakDensityParam(sampleGroups=c("1", "1")))
    
xset <- as(xset, "xcmsSet")
groupval(xset)

Error in round(groupmat[, "mzmed"], 1) : 
  non-numeric argument to mathematical function

Processing the same data files using old methods returns expected outpt.

xset_o <- xcms::xcmsSet(mzml_files, snthresh=10)
xset_o <- xcms::group(xset_o, method="density")

> groupval(xset_o)
          LCMS_1 LCMS_2 LCMSMS_1 LCMSMS_2
102.1/258      1   1029       NA       NA
103.1/144      2   1030       NA       NA
103.1/258      3   1031       NA       NA
104.1/49       4   1032     2056     2445
105.1/44       5   1033     2057     2446
106.1/41       6   1034     2058     2447

[andzajan]

I can see from git log on Bioconductor that Steffen @sneumann did add some commit on Saturday which didn't get into 3.9.x built, but went directly into 3.10.0.

commit 42cb2dc50d870b17c02c2121c331a07256638e06
Merge: 47f42c2 e72fa08
Author: Steffen Neumann <sneumann@ipb-halle.de>
Date:   Sat Apr 25 23:19:32 2020 +0200

    Merge pull request #468 from sneumann/fix/issue467

    ix issue #467 for fillPeaks() of an xcmsSet converted from an XCMSnSet

commit e72fa0850c4a813f911e4fb87d19fcf0e23982de
Author: Steffen Neumann <sneumann@ipb-halle.de>
Date:   Sat Apr 25 23:16:56 2020 +0200

    ix issue #467 for fillPeaks() of an xcmsSet converted from an XCMSnSet

[sneumann]

Hi, indeed, that is the same underlying issue.
The data structure of groupmat changed to an Array now,
and if you need the old behavior a patch is needed similar to the one
I have committed for fillPeaks. So would back-porting that help you ?
Yours, Steffen

[andzajan]

Hi Steffen, thank you for a fast response. It's not clear to me what do you mean by back-porting.
Do you mean creating a patch to mimic old groupval behavior? This would be my preference, do you want me to give it a go?

[andzajan]

Hi @sneumann, I have made an attempt to fix a problem.

andzajan@b6fb788

I did create an unit test file, to check if outputs are consistent for both xcmsSet and XCMSnExp objects. I did create new unit test file as I was not able to get existing tests which are using global environment objects to run on my local machine.

Can you have a look please? I don't want to open PR before you confirm that this is what you had in mind.

[jorainer]

This seems to be related to issue #464. @wkumler has already a fix for that ready, I'm just waiting for his PR on this.

[jorainer]

Can we close this issue now @andzajan ?

[andzajan]

Yes, seems to be solved now. Thank you!