feat: add filterSpectra function (issue #41)

[jorainer]

• Add a filterSpectra method that allows to filter the Spectra within an MsExperiment updating eventually present relationships (links) between samples and spectra.

[philouail]

So this ._SPECTRA_IDX variable already existed before this update ?

[jorainer]

Ah, no. but very good question! I specifically add this variable before I call the filter function on the Spectra object. That's the only way (at least that I found) how I can now to which spectra the data was filtered. So, the workflow is:

• add the spectra variable ._SPECTRA_IDX to the Spectra containing the index 1:length spectra (I chose this name for the spectra variable to not overwrite any potentially existing spectra variable with the same name - unlikely that somebody will call a spectra variable with this name - at least I assume)
• filter the Spectra object
• after filtering I need to update/consolidate the link (mapping) between samples and Spectra (which is stored in the @sampleDataLinks[["spectra"]] - it contains a two column matrix, the first with the index of the sample, the second with the index of the spectrum assigned to this sample). I can use the ._SPECTRA_IDX variable for that, because this got also subset (along with the Spectra object). So, I need to only keep rows in the above mapping matrix with the spectrum index (second column) present in ._SPECTRA_IDX.
• after that I remove the ._SPECTRA_IDX spectra variable again, because it's not needed anymore.

let me know if something is still unclear.

[philouail]

For me this looks great, I think it's a really nice method to have ! thanks

[lgatto]

General/design questions:

• why not using the actual Spectra filtering functions such as filterRt()... on an MsExperiment object, rather that having a general filterSpectra() method?
• why not (also) offer filterSpectra() for Spectra objects?

[lgatto]

... while keeping possibly present relationships between samples ...

[lgatto]

Possibly, not eventually.

[lgatto]

Why not in ProtGenerics? See general discussion point.

[jorainer]

Thanks for the review @lgatto ! Regarding the discussion points:

Why not in ProtGenerics? See general discussion point.

Totally agree - we should add it there.

why not using the actual Spectra filtering functions such as filterRt()... on an MsExperiment object, rather that having a general filterSpectra() method?

We had a discussion on that in issue #30 - I thought the conclusion from that was that we do not want to have these general filter functions for MsExperiment because we don't know (yet) how to handle the individual slots/data types within the object. A filterSpectra method would be more explicit - it clearly tells we're going to filter the Spectra within the object.

Also, this function would avoid having actual implementations of e.g. filterRt for MsExperiment objects. We can use any filter functions on a Spectra object with the filterSpectra method.

We could even have other implementations of filterSpectra that would use AnnotationFilter-like filtering, e.g. filterSpectra(mse, filter = ~ ms_level == 2L) or even combinations of filters filterSpectra(mse, filter = ~ ms_level == 2L & rt > 200) - such methods would then also make sense

So, summarizing:

• I'm OK to also implement filterRt, filterMsLevel etc for MsExperiment, but they would in the end all re-use the code from the filterSpectra (i.e. the one to keep/update the association between samples and spectra). Having a filterSpectra would just make already all filter methods for Spectra available immediately for MsExperiment, without the need to have actual implementations (+unit tests +documentation +more code to maintain).

• I'm also OK to have a filterSpectra for Spectra - but the only benefit I see there is the possibility to combine filters or express filters in a more natural way (such as ms_level == 2L).

happy to discuss further

[lgatto]

Thank you!

• OK to go for filterSpectra() to make it explicit that we filter the Spectra slot!
• I like the AnnotationFilter syntax (we have it in filterFeatures()). In you example, I'm just confused why ms_level and not msLevel (as in spectraVariables).
• If the filterSpectra() syntax proves useful and flexible, then I think it might be a useful addition to Spectra. Another benefit would be consistency between classes. What do you think? We could also wait a bit.

[jorainer]

A, yes, msLevel == 2L makes more sense. Agree also to add filterSpectra,Spectra,formula to Spectra - but maybe at a later stage. For now, if you agree, I would suggest to:

• move filterSpectra generic to ProtGenerics
• implement filterSpectra,MsExperiment,function in MsExperiment

work on filterSpectra,Spectra,formula and filterSpectra,MsExperiment,formula at a later stage.

[lgatto]

Yes, fine by me - thank you.

Please just change the eventually to possibly in the man page.

[lgatto]

I missed this one: eventually -> possibly.

[jorainer]

replaced now all "eventually" with "possibly". thanks!