-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some more Spectra methods #378
Conversation
- Add filterMsLevel,Spectra method. - Add coerce,Spectra,MSnExp method (issue #370). - Add related unit tests and documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
No change requests at all @sgibb ? for real? |
fd <- AnnotatedDataFrame(as.data.frame(mcols(from))) | ||
fromf <- as.character(unique(fromFile(from))) | ||
process <- new("MSnProcess", files = fromf, | ||
processing = paste("Data converted from Spectra:", date())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, IMHO its fine. There is only a small point with the processing message for the MSnExp
. We have a logging
function in R/utils.R where we use "[", date(), "]"
instead of date()
:
Lines 619 to 626 in 367b26f
logging <- function(object, msg, date. = TRUE) { | |
if (date.) | |
msg <- paste0(msg, " [", date(), "]") | |
object@processingData@processing <- | |
c(object@processingData@processing, msg) | |
if (validObject(object)) | |
return(object) | |
} |
But I have seen that we don't be consistent here:
Lines 89 to 90 in 367b26f
process <- new("MSnProcess", | |
processing = paste("No data loaded:", date())) |
Line 281 in 367b26f
processing = paste("Data loaded:", date()), |
Lines 23 to 24 in 70a9ec3
processing=paste("Quantitation data loaded:",date(), | |
" using readMSnSet."), |
Lines 66 to 69 in 57fb5db
object@processingData@processing <- c(object@processingData@processing, | |
paste(n," (",m, | |
") precursors (spectra) extracted: ", | |
date(),sep="")) |
Lines 88 to 92 in 57fb5db
object@processingData@processing <- c(object@processingData@processing, | |
paste("Curves <= ", | |
t | |
," set to '0': ", | |
date(),sep="")) |
In fact we more often use the date()
construct than "[", date(), "]"
or logging
:
$ grep -o "date()" *.R | wc -l
45
$ grep -o "logging(" *.R | wc -l
27
So I wasn't sure what to do and didn't mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't even aware of the "[", date(), "]"
- I used readMSData
as template.
"collisionEnergy", "ionisationEnergy", "precursorScanNum", | ||
"precursorMZ", "precursorCharge", "precursorIntensity", | ||
"mergedScan", "centroided", "spectrum") | ||
fdta <- fdta[, !colnames(fdta) %in% red_cn, drop = FALSE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't all feature variables passed along during coercion? I would prefer to keep all of them by default and have a selectFeatureData
function to select those to keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guessed because they are part of a Spectrum
object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not pass all feature variables to avoid redundancy. All these that are not passed along will be present (are set) in the individual Spectrum
object - so no need to have them in addition as metadata columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thank you. Is there already an accessor for these (centroided,Spectra
, ...)? The implementation will be identical to the corresponding in memory MSnExp
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all methods to access slot values for Spectrum
objects are also implemented for Spectra
objects (in methods-Spectra.R), returning always a vector of length equal to Spectra
with the corresponding slot values.
Add
filterMsLevel,Spectra
,coerce,Spectra,list
andcoerce,Spectra,MSnExp
(issue #370) andclean,Chromatograms
(issue #354).