Add support for OnDiskMSnExp in selectFeatureData #285
Conversation
- Add support for OnDiskMSnExp in selectFeatureData function (issue #283). - Add requiredFvarLabels function to return the minimal required columns in the featureData of an object. - Fix Spectrum1 and Spectrum2 constructor functions to accept also missing values for some columns/attributes. - Add related unit tests and documentation.
R/functions-OnDiskMSnExp.R
Outdated
| ## "retentionTime", "polarity", "msLevel", | ||
| ## "totIonCurrent", "originalPeaksCount", | ||
| ## "centroided") | ||
| reqCols <- .MSnExpReqFvarLabels |
There was a problem hiding this comment.
Minor point: Do you really need the new variable reqCols here? Beside the following line it is never used. Why do you not use .MSnExpReqFvarLabels[!(.MSnExpReqFvarLabels %in% colnames(x))]?
There was a problem hiding this comment.
nope, don't need reqCols - will remove.
R/functions-Spectrum1.R
Outdated
| @@ -102,60 +103,63 @@ Spectra1_mz_sorted <- function(peaksCount = NULL, rt = numeric(), | |||
| if (length(mz) != length(intensity)) | |||
| stop("Lengths of 'mz' and 'intensity' do not match!") | |||
| nvals <- length(nvalues) | |||
| empty_int <- rep(NA_integer_, nvals) | |||
| empty_num <- as.numeric(empty_int) | |||
| empty_log <- as.logical(empty_int) | |||
There was a problem hiding this comment.
I think we will lose the fight against snake_case 😉
There was a problem hiding this comment.
😄 no way - snake_case rules. I'll actually replace em all with 🐫 since the rest of the function uses also only CamelCase.
R/functions-Spectrum1.R
Outdated
| if (length(rt) == 0){ | ||
| rt <- rep(NA_integer_, nvals) | ||
| if (!length(rt)) { | ||
| rt <- empty_int |
There was a problem hiding this comment.
Are all this if(!length(...)) statements really necessary? I have no clue about the input values. Would it be possible to set the default argument of this constructor to e.g. rt=rep_len(NA_integer_, length(nvalues))?
There was a problem hiding this comment.
I know, it's ugly this way. The problem is that the upstream function uses e.g. fdata$polarity to pass the polarity column from the feature data to the function. If the featureData has been reduced and column "polarity" was removed, fdata$polarity is NULL and NULL is then passed with the polarity parameter.
R/utils.R
Outdated
| if (missing(fcol)) { | ||
| if (graphics) { | ||
| if (!requireNamespace("shiny")) | ||
| warning("The shiny package is required to use the graphical interface.") |
There was a problem hiding this comment.
IMHO this should be if (graphics && !requireNamespace("shiny")). Currently a warning is thrown if shiny is missing. Nevertheless the .selectShinyFeatureData is executed and will fail.
@lgatto MSnbase really suggests shiny just for fcol selection? MSnbase has already a lot of dependencies. Starting a browser and build a webpage for selecting some columns of a DataFrame is a little bit weird. I would suggest to remove this shiny part.
There was a problem hiding this comment.
We could move shiny from Suggests to Enhances and use your suggested requireNamespace. Most users will have shiny anyway installed.
There was a problem hiding this comment.
In some applications (mainly with MSnSet for spatial proteomics), large feature tables are really annoying, and a shiny app to easily select them is handy (we even have such a tab in our pRolocGUI apps).
@jotsetung - I don't think that Enhances is correct - it would imply that MSnbase enhances shiny (the meaning of Enhances has been confused and is confusing). Suggests is the right place.
There was a problem hiding this comment.
OK, I'll put it back to Suggests.
R/utils.R
Outdated
| varl <- character() | ||
| if (x == "OnDiskMSnExp") | ||
| varl <- .MSnExpReqFvarLabels | ||
| varl |
There was a problem hiding this comment.
varl needed? What about
if (match.arg(x) == "OnDiskMSnExp")
.MSnExpReqFvarLabels
else
character()
|
never ignoring any of your suggestions @sgibb ! thanks for reviewing. |
- Move feature data selection code to fdata-selection.R - Move related unit tests to test_fdata-selection.R
|
@jotsetung - Checking locally when realising that I think you forgot to |
|
Aaaah, sorry for that! I did push it now - too late |
featureData of an object.
values for some columns/attributes.