rformassspectrometry / Spectra

Low level infrastructure to handle MS spectra
https://rformassspectrometry.github.io/Spectra/
34 stars 24 forks source link

fix: bug in containsMz() when mz is not sorted (see #258) #259

Closed lgatto closed 1 year ago

jorainer commented 1 year ago

I'm wondering if this check should not go into the (backend) validator methods? That should/could be more efficient since the check would only called once instead of each time the function e.g. containsMz is called. AFAIK we require the m/z values to be sorted. The MsBackendDataFrame has this check (https://github.com/rformassspectrometry/Spectra/blob/master/R/MsBackendDataFrame-functions.R#L55:L63) - maybe we should add a validator method for the MsBackend class and add the sorted m/z check there?

lgatto commented 1 year ago

The last commit implements what what discussed in #258

lgatto commented 1 year ago
> sciex_file <- dir(system.file("sciex", package = "msdata"),
+                   full.names = TRUE)
+ sciex <- Spectra(sciex_file, backend = MsBackendMzR())
> x1 <- containsMz(sciex, mz = c(123.7, NA, 109.07), which = "any")
> x2 <- containsMz(sciex, mz = c(109.07, 123.7, NA), which = "any")
> x3 <- containsMz(sciex, mz = c(109.07, 123.7), which = "any")
> identical(x1, x2)
[1] TRUE
> identical(x1, x3)
[1] TRUE
lgatto commented 1 year ago

@jorainer - could you approve and/or merge, if you are OK with the PR