[Fix + Improvement] Faster _ecg_findpeaks_hamilton and bug-fix

[purpl3F0x]

Description

This PR aims to improve performanc of _ecg_findpeaks_hamilton and fix some probable bugs

Proposed Changes

Bug Fixes:

1. Changed possible bug popping from n_pks if len(n_pks) > 8, it should probably be len(n_pks)
2. Removed dead code of i += 1 at the end of the loop

Improvements:

1. changed loop iteration range to avoid checks inside the loop
2. use insort instead of appending& sorting in an already sorted array (tested about x300-500 times faster on arrays of 50 elements)
3. Use deques instead of manually pushing and popping to reduce constantly allocating new memory.

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

• I have read the CONTRIBUTING file.
• My PR is targeted at the dev branch (and not towards the master branch).
• I ran the CODE CHECKS on the files I added or modified and fixed the errors.
• I have added the newly added features to News.rst (if applicable)

[purpl3F0x]

Not sure about bug fix 1, but the current code doesn't seem logical.

Insort vs insert+sort

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2a1c621) 54.60% compared to head (b365588) 54.60%.

Files	Patch %	Lines
neurokit2/ecg/ecg_findpeaks.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #947      +/-   ##
==========================================
- Coverage   54.60%   54.60%   -0.01%     
==========================================
  Files         304      304              
  Lines       14332    14326       -6     
==========================================
- Hits         7826     7822       -4     
+ Misses       6506     6504       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danibene]

Hi @purpl3F0x , thank you for this! Could you explain what kind of "buggy" behavior you saw that motivated fix 1? It would be ideal if you could add a test https://github.com/neuropsychology/NeuroKit/tree/master/tests demonstrating that your fix works

[purpl3F0x]

Collaborator

Hello, the controversial code is pretty small. On this line

NeuroKit/neurokit2/ecg/ecg_findpeaks.py

Line 372 in 3d004b4

if len(n_pks) > 8:

There is a check for the length of the n_pks, but this part of the code is doing append/pop&on s_pks. Those 2 buffers behave similarly, holding the last 8 most recent peaks. They don't relate to each other. It is a typo, from the original repo the code was based from. There is a good possibility it works by accident cause at some point both array will top up and have legth of 8 elements. It's not a software bug, it's an implementation error.

EDIT:
I'm not very familiar with the algorithm but I think this part of the code is referring to this part of the paper: ( page 2 right column)
https://www.cinc.org/old/Proceedings/2002/pdf/101.pdf

The detection threshold used in 4 and 5 above is
calculated using estimates of the QRS peak and noise
peak heights. Every time a peak is classified as a QRS
complex, it is added to a buffer containing the eight most
recent QRS peaks. Every time a peak occurs that is not
classified as a QRS complex, it is added to a buffer
containing the eight most recent non-QRS peaks (noise
peaks). The detection threshold is set between the mean
or median of the noise peak and QRS peak buffers
according to the formula:
Detection_Threshold = Average_Noise_Peak +
TH*(Average_QRS_Peak - Average_Noise_Peak)
where TH is the threshold coefficient (generally between
0.3125 and 0.475). Similarly, the R-to-R interval
estimate used in 5 is calculated as the median or mean of
the last eight R-to-R intervals

[danibene]

I see, would you mind opening an issue on the original repository then too? I see that someone already opened an issue about the code at the end of the loop you removed berndporr/py-ecg-detectors#23

Until it's fixed, I think you could just add comments stating that the implementation differs slightly from the original code

[purpl3F0x]

I see, would you mind opening an issue on the original repository then too? I see that someone already opened an issue about the code at the end of the loop you removed berndporr/py-ecg-detectors#23

Until it's fixed, I think you could just add comments stating that the implementation differs slightly from the original code

I will also open an issue on the original repo. (berndporr/py-ecg-detectors#24)

If I'm not mistaken the way this "works", now is with s_pks having a max length depending on the input signal, until the length of n_pks becomes 8. I think that's what called an undefined behavior 😁.

[purpl3F0x]

The issue has been confirmed and fixed on the original repo as of berndporr/py-ecg-detectors@f5d949d

[berndporr]

Thanks guys. I've applied the fix also to my repo. I had to apply it manually from the diff as you have linted your version but should be in syn now again.

[danibene]

This looks good to me, thanks @purpl3F0x & @berndporr !