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

New peakshape #137

merged 5 commits into from Dec 6, 2018

New peakshape #137

merged 5 commits into from Dec 6, 2018


Copy link

@Bisaloo Bisaloo commented Dec 2, 2018

This PR proposes a completely different algorithm to find the FWHM.

  • The old algorithm estimated the FWHM by finding the closest values to the half maximum value. This meant that it could easily make mistakes when spectra had multiple peaks (see plots below).

    The new algorithm starts from the maximum value and then moves left (then right) until it finds the first value that falls below the half maximum value. This means it will never be fooled by multiple peaks

(left plot is the old algorithm and right plot is the new one)

  • With the old algorithm, it was very had to determine when the FWHM overlapped with the wavelength range. This means that the right-hand side value (HWHM.r) was not actually at half maximum but was the lowest value to the right of the FWHM.

    With the new algorithm, it's trivial to detect such cases and peakshape() now accordingly returns NA when the FWHM range overlaps with the wavelength range.

(left plot is the old algorithm and right plot is the new one)

peakshape(sicalis, select = grepl("T$", colnames(sicalis)))
ind1.T 18.38233 653 NA 145 NA
ind2.T 17.66600 681 NA 180 NA
ind3.T 14.53060 671 NA 165 NA
ind4.T 15.12063 660 NA 151 NA
ind5.T 17.49428 627 NA 123 NA
ind6.T 18.02172 632 NA 129 NA
ind7.T 16.76935 672 NA 164 NA
  • The new algorithm is slower (spends 150% of the time spent before) than the old one but I may still be able to improve it a bit (fixed by ffeb760)
expr min lq mean median uq max neval cld
{ pavo::peakshape(sicalis, plot = FALSE) } 4.736414 4.813911 5.634903 4.873461 5.059997 186.11863 1000 a
{ peakshape(sicalis, plot = FALSE) } 4.697731 4.775466 5.324652 4.831968 4.966489 15.28568 1000 a

@Bisaloo Bisaloo force-pushed the new-peakshape branch 3 times, most recently from 0eece4a to 85f54f2 Compare Dec 2, 2018
@Bisaloo Bisaloo changed the title [WIP] New peakshape New peakshape Dec 4, 2018
@Bisaloo Bisaloo closed this Dec 4, 2018
@Bisaloo Bisaloo reopened this Dec 4, 2018
Copy link

thomased commented Dec 5, 2018

This looks awesome. Want to throw a few tests in if you get a chance? It's one of the last entirely-uncovered bits :)

Copy link
Collaborator Author

Bisaloo commented Dec 5, 2018

Sure! I'll probably have a go in a couple of days!

@thomased thomased merged commit e4d304f into rmaia:mee-fixes Dec 6, 2018
1 check passed
@Bisaloo Bisaloo deleted the new-peakshape branch Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants