NeuroKit icon indicating copy to clipboard operation
NeuroKit copied to clipboard

RPeaks are calculated twice in ecg_process

Open CReizner opened this issue 2 years ago • 6 comments

Within the function ecg_process, you are calculating rpeaks with ecg_peaks but do not pass it to the ecg_quality function and _ecg_quality_averageQRS will therefore calculate the rpeaks list again.

def ecg_process(ecg_signal, sampling_rate=1000, method="neurokit"):
    ...
    instant_peaks, rpeaks, = ecg_peaks(
        ecg_cleaned=ecg_cleaned, sampling_rate=sampling_rate, method=method, correct_artifacts=True
    )
    ...
    quality = ecg_quality(ecg_cleaned, !!!rpeaks=None!!!, sampling_rate=sampling_rate)
    ...

CReizner avatar Jan 10 '22 14:01 CReizner

Hi 👋 Thanks for reaching out and opening your first issue here! We'll try to come back to you as soon as possible. ❤️ kenobi

welcome[bot] avatar Jan 10 '22 14:01 welcome[bot]

Hi @CReizner thanks for catching this! Indeed you're right, it should be rpeaks=rpeaks rather than rpeaks=None in ecg_process to avoid peak computation again. Would you like to open a pull request to the dev branch here?

zen-juen avatar Jan 11 '22 06:01 zen-juen

Hi @zen-juen. It should be rpeaks=rpeaks['ECG_R_Peaks'], because it wants a list. I honestly don't know how git or github works, so I don't see myself creating a pull request =D

CReizner avatar Jan 11 '22 12:01 CReizner

No worries, we'll fix it then. Thanks a lot for letting us know!

DominiqueMakowski avatar Jan 11 '22 12:01 DominiqueMakowski

This issue has been automatically marked as inactive because it has not had recent activity. It will eventually be closed if no further activity occurs.

stale[bot] avatar Jul 13 '22 12:07 stale[bot]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Sep 20 '22 19:09 stale[bot]