Add peak variable support to MsBackendMemory and MsBackendDataFrame #297

jorainer · 2023-06-15T12:38:58Z

This PR adds the peak variable functionality mentioned in issues #289 (and #287). In essence the promise of peaksData is to return a 2 dimensional data structure. Can be a matrix or data.frame. If only "mz" and "intensity" are requested MsBackendMemory and MsBackendDataFrame return a list of matrix, otherwise a list of data.frame.

- `peaksData,MsBackendMemory` returns by default a `list` of `matrix` or a `list` of `data.frame`s if other peak variables than `"mz"`, `"intensity"` are requested. Issue #289.

- Ensure lazy evaluation of processing queue in `peaksData,Spectra` does not break if requested `columns` do not contains `"mz"` and `"intensity"`. Issue #289.

- `applyProcessing` ensures that all peaks variables are properly updates/subset depending on the processing queue (issue #289).

- `spectraData<-,Spectra` and `$<-,Spectra` throw an error if processing queue is not empty and values for peaks variables are requested to be replaced. Issue #289.

- Add support for peak variables to `MsBackendDataFrame`. Issue #289. - Add examples for peak variables to `MsBackend` and `Spectra` documentation. - Expand documentation for peaks variables to the `Spectra` vignette.

- Fix issue in `MsBackendMemory` that allowed to have only either m/z or intensity as a peaks variable.

- Add support for peaks variables to `MsBackendDataFrame` and add/test its funcitionality (issue #289).

lgatto · 2023-06-16T13:58:21Z

NEWS.md

+- Add `combinePeaks` generics.
+- Add `combinePeaks,Spectra` to combine peaks within each spectrum in a
+  `Spectra`.
+>>>>>>> main


fixed now :)

lgatto · 2023-06-16T20:36:39Z

R/MsBackendDataFrame-functions.R

@@ -69,6 +69,26 @@ NULL
    NULL
 }

+.peaks_variables <- function(x) {
+    if (x@version != "0.1") {


Did we not agree to check for the presence of the peaksVariablesslot here?

lgatto · 2023-06-19T15:38:06Z

R/MsBackendMemory.R

+                                             columns = c("mz", "intensity")) {
+        if (length(object)) {
+            cns <- colnames(object@peaksData[[1L]])
+            if (length(columns) == length(cns) && all(cns == columns))


Would this not fail if I set peaksData(..., columns = c("intensity", "mz"))? Should cns and columns not be sorted in the last part of the if?

The aim of that if is that @peaksData is only returned if columns is equal to the colnames of the @peaksData matrices, i.e. only if columns = c("mz", "intensity) and colnames of @peaksData is c("mz", "intensity"). If columns is e.g. c("mz", "intensity", "peak_ann") or it is c("intensity", "mz") it should be FALSE to ensure that we combine the @peaksData with the @peaksDataFrame or we iterate over @peaksData to re-order the columns.

So, sorting would not be correct here because then the order of columns in peaks data would be e.g. "mz", "intensity" even if the user requested "intensity", "mz". MsBackend should allow to extract peaks data matrices in any user requested order.

lgatto · 2023-06-19T15:41:01Z

R/Spectra.R

+#'   `data.frame`), with each array providing the values for the requested
+#'   *peak variables* (by default `"mz"` and `"intensity"`). Optional parameter
+#'   `columns` is passed to the backend's `peaksData` function to allow
+#'   selection of specific (or additional) peaks variables (columns) that


... allow the selection ...

lgatto · 2023-06-19T15:44:20Z

R/Spectra.R

+setMethod(
+    "spectraData", "Spectra",
+    function(object, columns = spectraVariables(object)) {
+        if (length(object@processingQueue) &&


Just a suggestion, but I would find it useful to have some comments that describe the different situations here.

you mean within the code?

I added comments in the code. Basically, if the processing queue is empty or the user does not request peak variables with columns the spectraData method from the backend is called. Otherwise we need to execute the processing queue (calling peaksData on the Spectra object) to ensure that e.g. peaks are filtered etc and add the individual peaks variables to the returned DataFrame.

lgatto · 2023-06-19T15:47:10Z

For now, additional peaks variables are only possible for in-memory (i.e. Memory and DataFrame) backends, right? Additional ones could come (for example SQL-based backends), but not (possibly never) for the mzR backend. This could be mentioned/discussed either in the manual page and/or in the vignette.

jorainer · 2023-06-20T13:31:44Z

For now, additional peaks variables are only possible for in-memory (i.e. Memory and DataFrame) backends, right?

Actually, no. Since e.g. MsBackendMzR and others inherit from MsBackendDataFrame (or MsBackendMemory) they support also peaks variables. For MsBackendMzR they would obviously not (yet) imported from mzML files directly, but it would be possible to add them later (with a future addPeaksVariable function). There is already some notes on peaks variable support of different backends in the (too large) documentation of Spectra objects. But that could then also be improved in the revamp of the docs (issue #288 )

jorainer · 2023-06-20T15:50:18Z

Thanks @lgatto for the review. I have now addressed the comments.

lgatto · 2023-08-25T10:32:13Z

R/MsBackend.R

-#' `MsBackendMemory` supports also arbitrary peak annotations while the
-#' `MsBackendDataFrame` does not have support for such additional peak
-#' variables.
+#' `"intensity"` is fastest for the `MsBackendMemory`.


I think there's a missing ) after "intensity".

lgatto · 2023-08-25T10:33:51Z

R/MsBackend.R

+#' The `peaksData` function for `MsBackendMemory` and `MsBackendDataFrame`
+#' returns a `list` of `numeric` `matrix` by default (with parameter
+#' `columns = c("mz", "intensity")`). If other peak variables are requested,
+#' a `list` of `data.frame` is returned (to ensure m/z and intensity values


Not sure to understand the "to ensure m/z ...". Should it not read "ensuring that m/z ..."?

yes, thanks

lgatto · 2023-08-25T10:39:07Z

R/MsBackendDataFrame.R

              if (missing(data)) data <- DataFrame()
              if (is.data.frame(data))
                  data <- DataFrame(data)
              if (!is(data, "DataFrame"))
                  stop("'data' has to be a 'DataFrame'")
+              peaksVariables <- intersect(peaksVariables, colnames(data))
+              if (sum(c("mz", "intensity") %in% peaksVariables) == 1L)


If I understand this check, it will throw an error if only one of "mz" or "intensity" are in the peaksVariables. If none are, this wouldn't trigger the error. Why not

sum(c("mz", "intensity") %in% peaksVariables) != 2L

good catch! thanks, changed that.

actually, after checking again: we were supporting the following situations:

user provides m/z and intensity

user does not provide neither m/z and intensity (in which case 0-length spectra are available, but the Spectra is still considered valid).

An error is only thrown if either m/z but not intensity, or intensity but not m/z is provided. from a data point of view that does not make sense, you can't have one of the two. you could either have both or none.

Open to discuss @lgatto when you think that's not a good idea.

OK, fine by me. May be just clarify with a comment?

jepp, will add that and also check that I mention it in documentation

added as a comment and docu.

lgatto · 2023-08-25T10:41:22Z

R/MsBackendMemory.R

+                                             columns = c("mz", "intensity")) {
+        if (length(object)) {
+            cns <- colnames(object@peaksData[[1L]])
+            if (length(columns) == length(cns) && all(cns == columns))


jorainer added 10 commits May 25, 2023 15:33

refactor: peaksData,MsBackendMemory can return data.frame

6556a26

- `peaksData,MsBackendMemory` returns by default a `list` of `matrix` or a `list` of `data.frame`s if other peak variables than `"mz"`, `"intensity"` are requested. Issue #289.

refactor: peaksData,Spectra to correctly filter peaks matrix

f0d13c6

- Ensure lazy evaluation of processing queue in `peaksData,Spectra` does not break if requested `columns` do not contains `"mz"` and `"intensity"`. Issue #289.

refactor: $,Spectra correctly returns peak variable

dc02d60

refactor: applyProcessing correctly manages processing queue

533a225

- `applyProcessing` ensures that all peaks variables are properly updates/subset depending on the processing queue (issue #289).

ci: install development version of remotes

8ee8335

refactor: spectraData<- and $<- to check peaks variables

1482b9d

- `spectraData<-,Spectra` and `$<-,Spectra` throw an error if processing queue is not empty and values for peaks variables are requested to be replaced. Issue #289.

refactor: add support for peak variables to MsBackendDataFrame

9b3fc3a

- Add support for peak variables to `MsBackendDataFrame`. Issue #289. - Add examples for peak variables to `MsBackend` and `Spectra` documentation. - Expand documentation for peaks variables to the `Spectra` vignette.

fix: MsBackendMemory requires both mz and intensity peaks variables

7b2d2e8

- Fix issue in `MsBackendMemory` that allowed to have only either m/z or intensity as a peaks variable.

refactor: extend support for peaks variables (issue #289)

db17225

tests: add peaks variables unit tests for MsBackendDataFrame

a6fabdf

- Add support for peaks variables to `MsBackendDataFrame` and add/test its funcitionality (issue #289).

jorainer requested a review from lgatto June 15, 2023 12:39

Merge branch 'main' into jomain

b5122c9

lgatto requested changes Jun 16, 2023

View reviewed changes

fix: remove merge conflict marker

2f21f2d

jorainer requested a review from lgatto June 16, 2023 19:27

lgatto reviewed Jun 19, 2023

View reviewed changes

refactor: address Laurent's comments

ef17af4

fix: use .hasSlot instead of slotNames

1daa730

jorainer requested a review from lgatto June 20, 2023 15:50

jorainer requested a review from sgibb June 28, 2023 05:20

jorainer added 3 commits June 30, 2023 15:07

Merge remote-tracking branch 'origin/main' into jomain

010638c

Merge branch 'main' into jomain

a5b8a7f

Merge branch 'main' into jomain

3db4369

lgatto reviewed Aug 25, 2023

View reviewed changes

fix: address Laurent's comments

1645a4c

lgatto self-requested a review September 22, 2023 11:54

docs: add comment and documentation

7d169e4

lgatto approved these changes Sep 22, 2023

View reviewed changes

jorainer merged commit 585abb1 into main Sep 22, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add peak variable support to MsBackendMemory and MsBackendDataFrame #297

Add peak variable support to MsBackendMemory and MsBackendDataFrame #297

jorainer commented Jun 15, 2023

lgatto Jun 16, 2023

jorainer Jun 16, 2023

lgatto Jun 16, 2023

jorainer Jun 20, 2023

lgatto Jun 19, 2023

jorainer Jun 20, 2023

lgatto Aug 25, 2023

lgatto Jun 19, 2023

lgatto Jun 19, 2023

jorainer Jun 20, 2023

jorainer Jun 20, 2023

lgatto commented Jun 19, 2023

jorainer commented Jun 20, 2023

jorainer commented Jun 20, 2023

lgatto Aug 25, 2023

lgatto Aug 25, 2023

jorainer Sep 22, 2023

lgatto Aug 25, 2023 •

edited

Loading

jorainer Sep 22, 2023

jorainer Sep 22, 2023

lgatto Sep 22, 2023

jorainer Sep 22, 2023

jorainer Sep 22, 2023

lgatto Aug 25, 2023

Add peak variable support to MsBackendMemory and MsBackendDataFrame #297

Add peak variable support to MsBackendMemory and MsBackendDataFrame #297

Conversation

jorainer commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgatto commented Jun 19, 2023

jorainer commented Jun 20, 2023

jorainer commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgatto Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgatto Aug 25, 2023 •

edited

Loading