Skip to content
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

Possible bug in descendPeaks #583

Closed
Pascallio opened this issue Oct 28, 2022 · 3 comments
Closed

Possible bug in descendPeaks #583

Pascallio opened this issue Oct 28, 2022 · 3 comments

Comments

@Pascallio
Copy link
Contributor

Dear developers,

I might have encountered a bug for an unlikely, but possible situation in MSnbase:::descendPeaks. According to the documentation, "the peak region is defined by descending from the identified centroid/peak on both sides until the measured signal increases again.". However, in situations where the centroid is either the first or last signal in a spectrum, the other side of the peak seems to be ignored.

Some example code for reproducibility:

mz <- c(100.14, 100.145, 100.15) # masses
intensity <- c(1000, 700, 600) # intensities
peakIdx <- 1 # position of the centroid

MSnbase:::descendPeak(mz = mz, intensity = intensity, peakIdx = peakIdx) 

This results in a mass of 100.14 and does not take other values than the centroid into account (which in this example would have resulted in 100.1441). It seems that MSnbase:::descendPeak calculates the half_width, which in this case results in 0 and therefore only considers the centroid itself.

Is this the correct behaviour or should the remaining signals be taken into account until the (first) observed signal rises again?

Best,
Pascal

@jorainer
Copy link
Collaborator

Dear Pascal! Thanks for the pointing this out! Would you be willing to provide a pull request that fixes this issue?

Pascallio added a commit to Pascallio/MSnbase that referenced this issue Nov 20, 2022
This fix adds the parameter `descendIfEdge`, which changes the behavior of `descendPeak` if set to TRUE. Doing so will continue the descend if the centroid is near either end of the spectrum by trying the full width instead of the half width. Defaults to FALSE to keep the current behavior to preserve current centroid results.
@Pascallio
Copy link
Contributor Author

Hi Johannes,

Yes ofcourse, I've just written a fix and provided the pull request.

Cheers,
Pascal

lgatto added a commit that referenced this issue Dec 21, 2022
@lgatto
Copy link
Owner

lgatto commented Dec 21, 2022

I have merged and pushed to Bioc in devel and release. Thank you very much @Pascallio !

@lgatto lgatto closed this as completed Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants