Repository navigation
Include the right edge in find_local_peaks window - #573
Merged
Merged
Conversation
Member
|
Thanks, this looks good. On the XQRS side, I think the small sensitivity change you measured is acceptable given that the function is now correct. A few small requests:
|
tompollard
added a commit
that referenced
this pull request
Oct 31, 2026
`correct_peaks` is meant to search `search_radius` samples on both sides of each peak, but `shift_peaks` slices `sig[ind - r : min(ind + r, sig_len - 1)]`. That leaves out `ind + r`, and the last sample of the signal, so a peak there can't be found: ```python sig = np.zeros(30); sig[13] = 5; sig[29] = 5 processing.correct_peaks(sig, np.array([10]), search_radius=3, smooth_window_size=9, peak_dir="up") # [7], expected [13] processing.correct_peaks(sig, np.array([27]), search_radius=5, smooth_window_size=9, peak_dir="up") # [22], expected [29] ``` **Fix:** slice up to `min(ind + r + 1, sig_len)`. **Effect on record 100** (`correct_peaks` on the reference beats, XQRS and GQRS, radius 10, 36 and 61, all four `peak_dir` options): - With the radius used in the existing test (61), only one peak moves, and only for `peak_dir="down"`: the last beat, now on the final sample. - With radius 10 on GQRS beats (`"up"`, `"both"`, `"compare"`), almost every peak moves one sample right, from `ind + 9` to `ind + 10`. In all 2271 cases the new sample is higher in the smoothed-difference signal, so the old window was cutting off the maximum. - The existing `test_correct_peaks` expectations are unchanged. - XQRS and GQRS don't call `correct_peaks`, so their detections are unchanged. This is the same kind of off-by-one as #572 / #573, but in `shift_peaks` rather than `find_local_peaks`. The two PRs touch different functions and don't depend on each other. **Tests:** a peak exactly `search_radius` after the guess, and a peak on the last sample. The test fails on `main`. `pytest -m "not network"` and the docs build pass.
The slices stopped at i + radius - 1, so the sample radius positions to the right was never compared. Use one loop over the whole signal; the third loop was unreachable (MIT-LCP#474). The test classes in test_processing.py were named test_*, which pytest does not collect, so rename them to make those tests run. Fixes MIT-LCP#572
When several samples share the maximum value, the first of them is classified as the peak, not the middle one. A flat top wider than the radius gives a peak every <radius> samples along it.
The built-in max() iterates over the window one element at a time. ndarray.max() makes find_local_peaks about 3 times faster (XQRS on record 100 goes from 1.1 s to 0.4 s), with identical results for finite signals. Next to a NaN value, the window's maximum is now consistently NaN, so no peak is found there, where max() gave a result that depended on the position of the NaN. Lists are converted to arrays, so they still work.
tompollard
force-pushed
the
fix-find-local-peaks-window
branch
from
October 31, 2026 21:16
a07fc39 to
4df94ba
Compare
Member
|
Thanks for the fix! I went ahead and addressed the points above:
If you have local changes, please reset your branch to this one before pushing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #572. The window now covers
radiussamples on both sides like the docstring says, and the third loop that could never run is gone (#474).test_processing.py wasn't being collected because the classes were named
test_*, so I renamed them. The new test fails on main.XQRS uses
find_local_peaksfor its candidates, so its output shifts a little. On MIT-BIH 100, 101, 105, 108, 119, 200, 203, 207 and 208 it finds 21 fewer of 20856 beats (sensitivity 0.9777 to 0.9766), mostly in 208, and false positives stay at 733. If you'd rather keep XQRS as it was I can look at that side too.