-
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 filterPrecursorScan,MSnExp-method. #288
Conversation
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.
Nice PR @sgibb. One minor comment from my side.
R/methods-filters.R
Outdated
|
||
setMethod("filterPrecursorScan", "MSnExp", | ||
function(object, scanIndex, ...) { | ||
object <- object[.filterSpectraHierarchy(fData(object), scanIndex)] |
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.
This method will never work for MSnExp
objects, because they don't have the required columns in the feature data. Any plans to implement it for MSnExp
objects? A simple helper function that iterates through the Spectrum
objects to extract the acquisitionNum
and precursorScanNum
and returns a data.frame
might do 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.
Currently I don't have any plans to add such a function (because most MSnExp
won't have more than one msLevel
) but I used MSnExp
to provide this option.
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.
Right, forgot about that.
This all looks good. I would like to do two things before merging
@sgibb - Re 1. above, do you know if it's possible to re-process a raw file to fix the missing precursor data? Otherwise, I will try to get a new file (and hope we have a recent Xcalibur version). |
@lgatto Sorry, but I never used Putting this code into the vignette would be great (but you are right, we need an example file). What about the argument name |
@sgibb - I can get a file processed with Xcallibur 3.x tomorrow.
Indeed, this is confusing (I had to double check myself when testing yesterday). I think it should be |
… into issue282-spectraHierarchy
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.
OK from my side
Re |
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.
As mentioned in a comment, it would be nice to
- have an example file with MS3 data for some more testing - I should get one soon;
- add a section in the vignette that shows how to use the code and visualise to respective peaks using the MS3 example.
I'll approve and we might want to open a new issue for the points above.
I have now added an example that uses an MS3 TMT11. |
This PR closes #282. Briefly it adds a method to filter linked spectra (MS1 and its corresponding MS2 and if available their corresponding MS3 and so on). The information is available in the
featureData
via theprecursorScanNum
column.I use two
for
loops for the up/downward search in the hierarchy because recursion is slow in R and constructing the family tree would be more time consuming than the filtering.I am wondering how to handle the argument
scanIndex
infilterPrecursorScan
. In fact it is not thescanIndex
but theacquisitionNum
. Should I change the name or is there a way to translate between the two different indices?