search

rformassspectrometry / Spectra

Low level infrastructure to handle MS spectra
https://rformassspectrometry.github.io/Spectra/
34 stars 24 forks source link

Add filterPrecursorMaxIntensity and filterPrecursorIsotopes functions #290

Closed AharoniLab closed 1 year ago

AharoniLab commented 1 year ago

This PR adds the filterPrecursorMaxIntensity (issue https://github.com/rformassspectrometry/Spectra/issues/280) and filterPrecursorIsotopes (issue https://github.com/rformassspectrometry/Spectra/issues/281) functions along with documentation and unit tests.

jorainer commented 1 year ago

@lgatto , we addressed the comments. An example for estimatePrecursorIntensities comparing them against those defined by the manufacturer's software is given in the example section for that function. Also, a unit test was already in place before.

Note that the failing unit test for linux is due to a bug/problem in the remotes package. This will be fixed in the next release of the remotes package.

jorainer commented 1 year ago

After discussion with @lgatto we agreed to add the filterPrecursorMaxIntensity function. For the filterPrecursorIsotopes some more discussion is needed whether it would not make more sense to add that to e.g. xcms or MetaboAnnotation instead. That depends how common such data (with precursor isotopes) actually is.

AharoniLab commented 1 year ago

Hi Johannes,

I cannot follow the logic of splitting these very related function between packages. I can only say that the phenomena of precursor (and fragment) isotopic ions occurs always in DIA-MS methods and quite often also in DDA data (depending on instrument type, MS method settings etc.). Should a "filter isotopes" function be added to 'xcms'? Absolutely, but that in my view should be decoupled from the need for such a function in annotating Spectra because there can be other application workflows.

Nir

From: Johannes Rainer @.***> Sent: Wednesday, June 28, 2023 7:23:10 AM To: rformassspectrometry/Spectra Cc: Nir Shachaf; Author Subject: Re: [rformassspectrometry/Spectra] Add filterPrecursorMaxIntensity and filterPrecursorIsotopes functions (PR #290)

After discussion with @lgattohttps://github.com/lgatto we agreed to add the filterPrecursorMaxIntensity function. For the filterPrecursorIsotopes some more discussion is needed whether it would not make more sense to add that to e.g. xcms or MetaboAnnotation instead. That depends how common such data (with precursor isotopes) actually is.

— Reply to this email directly, view it on GitHubhttps://github.com/rformassspectrometry/Spectra/pull/290#issuecomment-1610758243, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADZL6DHOKG3V5O5KCBRC2Z3XNO5T5ANCNFSM6AAAAAAYKJVAJQ. You are receiving this because you authored the thread.Message ID: @.***>

jorainer commented 1 year ago

For whatever reason this PR is not picking up the recent commits that fix the merge conflicts. I will close this PR and open a new one instead.