DescendPeaks in C++#104
Conversation
This is the C++ implementation of the descendPeaks function of MSnbase. It takes a slightly different approach to the original by first subsetting the maximum region to consider. Next for each left/right descend, it checks if the next signal is A) above the signal percentage threshold and B) not higher than the previous signal (rising). If the signal passes both tests, the left/right index is updated. After both descends, these indexes are used to define the region of which the weighted mean of the mass is taken.
|
Just a quick comment - it would also be important to have unit tests to check that the results give expected results on small data or/and same as an R implementation on a more complex data. |
|
@Pascallio Thanks for your contribution. It is very welcome! Maybe a stupid question/suggestion. We try to keep the dependencies as low as possible (Suggests contains already a large list of dependencies). Currently we successfully avoid |
| bool goodSignal = current / centroidValue > signalRatio; | ||
|
|
||
| // Compare current to previous value to check for a rising signal | ||
| bool risingSignal = current >= previous; |
There was a problem hiding this comment.
Would this be a good test? Already a small plateau (2 indices of the same intensity value) would break here, even if it goes down after the overnext index.
There was a problem hiding this comment.
I do believe that is the default behaviour in the original MSnbase version here: https://github.com/lgatto/MSnbase/blob/1338323345d2a6cf856f1819dfb365300ab6d9d5/R/functions-Spectrum.R#L781-L788
I'm not sure about the current version in MsCoreUtils, but I could implement the stopAtTwo behaviour or perhaps stop at any given k if desired?
Avoids looping over the intensity vector twice Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Allows for setting a different maxK, which determines the maximum region the descendPeak algorithm is applied to
Thank you @sgibb! I fully understand the concern about dependencies. I don't have much experience with Rcpp, so how do I ensure it would end up in As for implementing it in pure C, I have no experience with C so it might take me a while to submit a pure C implementation but I'm willing to give it a shot when I have some spare time. |
|
Not sure if this PR is still relevant, but I would like to see a unit test, that compares the R- and C-level results. |
3 participants
This is the C++ implementation of the descendPeaks function of MSnbase. It takes a slightly different approach to the original by first subsetting the maximum region to consider. Next for each left/right descend, it checks if the next signal is A) above the signal percentage threshold and B) not higher than the previous signal (rising). If the signal passes both tests, the left/right index is updated. After both descends, these indexes are used to define the region of which the weighted mean of the mass is taken.
It is recommended to sanatize all inputs first in R before calling this function to ensure the right values are used, e.g. C-indexes, not R-indexes.