Fix issue with subset-based peakGroups alignment and other updates #703
Conversation
- Ensure to disable chunk-wise processing for all peaksData calls by passing parameter f = factor().
- Add the `filterMsLevel` methods for `MsExperiment` and `XcmsExperiment`.
- Split the PeakGroups alignment function to enable alignment using a predefined peak matrix.
- Fix issue #702 to ensure peakGroups subset-based alignment correctly uses the `minFraction` parameter. - Split the code for peakGroups-based alignment to enable alignment using externally defined retention time matrix of anchor peaks.
|
Note: unit tests fail until Spectra is updates. |
- Ensure the "ms_level" column in the `chromPeaksData` is of type `integer` to avoid obscure validity errors on extracted `Chromatogram` objects (happening randomly).
- Support alignment with the peakGroups method using a predefined anchor matrix.
- `.match_last` was returning a wrong result if used with `nomatch = -1`. This affected removing or adding elements to the processHistory during xcms preprocessing.
- Throw an error for `adjustRtime` if alignment results are already present.
|
OK. unit tests fixed - ready for review/comments @sneumann |
| @@ -20,6 +20,14 @@ setMethod("filterMz", "MsExperiment", | |||
| filterMzRange(object, mz, msLevel.) | |||
| }) | |||
|
|
|||
| #' @rdname XcmsExperiment | |||
| setMethod("filterMsLevel", "MsExperiment", | |||
| function(object, msLevel. = uniqueMsLevels(object)) { | |||
There was a problem hiding this comment.
First time I see a parameter with trailing dot. Is there a pattern I need to learn ?
There was a problem hiding this comment.
Yes, that's rather unusual, I agree. It's still from MSnbase days, without the . R was complaining about recursive arguments - I guess that came from the R dispatcher that got confused by a parameter msLevel and a msLevel method. For consistency and backward compatibility we used this also in other method definitions in Spectra. Thanks to the partial parameter matching from R users can use msLevel = 1L as well as msLevel. = 1L.
I know it looks ugly, but we can't change it here, because that comes from far upstream...
| " than 2x. This is dangerous and the algorithm is probably ", | ||
| "overcorrecting your data. Consider increasing the span ", | ||
| "parameter or switching to the linear smoothing method.") | ||
| rtime_adj |
There was a problem hiding this comment.
I like a more explicit return(rtime_adj), unless we use a style convention that suggests brevity rulez
There was a problem hiding this comment.
I also used return() a lot - but then had a chat with Martin (Morgan) and he mentioned that for R there's no need to have yet another function call at the end of a function since the last variable will anyway be returned by default. return should only be used e.g. to break within a function and still return something. That seems to be now the R coding style.
R/do_adjustRtime-functions.R
Outdated
| @@ -485,6 +393,10 @@ do_adjustRtime_peakGroups_orig <- function(peaks, peakIndex, rtime, | |||
| #' @details This function is called internally by the | |||
| #' do_adjustRtime_peakGroups function and the retcor.peakgroups method. | |||
| #' | |||
| #' Update for version 4.1.2: correctly consider the `sampleIndex` and | |||
There was a problem hiding this comment.
yes sure, thanks! will fix.
data/faahko_sub2.RData
Outdated
There was a problem hiding this comment.
Good question. This basically is "just" a small object to have simpler examples (i.e. load the data without having to first perform all preprocessing to then just run the example).
IMHO the faahKO should "only" contain the raw data and a object of a class from xcms should be in xcms - that way faahKO is independent of xcms and we avoid eventual cyclic redundancies.
data/xmse.RData
Outdated
There was a problem hiding this comment.
Same here, IMHO msdata should contain raw data files and be independent of data structures.
tests/testthat/test_Chromatogram.R
Outdated
| @@ -1,5 +1,5 @@ | |||
| ## test_that("extractChromatograms is deprecated", { | |||
There was a problem hiding this comment.
Not only deprecated, but gone. So test can go out of the window as well
There was a problem hiding this comment.
yep - will remove.
This fixes issue #702 and in addition splits the peakGroups alignment function into two parts (finally) supporting also the use of externally defined alignment matrices.