neuropsychology / NeuroKit

NeuroKit2: The Python Toolbox for Neurophysiological Signal Processing
https://neuropsychology.github.io/NeuroKit
MIT License
1.58k stars 421 forks source link

[Fix] Return an empty array rather than throwing an exception if no QRS dat… #1007

Closed ajb5d closed 4 months ago

ajb5d commented 4 months ago

Description

This PR aims fixes an error where running ecg_peakdetect on a signal without any QRS complexes would throw an exception rather than returning an empty array. There are a couple of issues (mostly closed due to inactivity) that relate to this: #580, #472, #134

Proposed Changes

I changed the _ecg_findpeaks_neurokit() function so that the length of beg_qrs is checked before accessing the first element. If the array is empty then return an empty array rather than triggering a ValueException.

Checklist

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

welcome[bot] commented 4 months ago

Thanks for opening this pull request! We'll make sure it's perfect before merging 🤗 force Make sure to read the contributing guide. Also, if you think that your contribution is worthy of it, you can consider adding yourself to the Contributors list (feel free to ask us if you have any doubts).

DerAndereJohannes commented 4 months ago

I see that you only put it on the neurokit method of the function i.e., _ecg_findpeaks_neurokit. Have you tested the other methods to see if they have the same result as your new version for neurokit? It would be nice to have consistent behaviour.

ajb5d commented 4 months ago

@DerAndereJohannes @danibene I added tests for the other methods (at least checking that they don't crash when run on empty input). I made a couple of other fixes to ensure that most methods handle empty input. Two small things not addressed (But I wanted to record for posterity):

ajb5d commented 4 months ago

I think the failed test is related to visbilitygraph which has a numpy dependency issue (we don't require a specific numpy version, the scipy > 1.13 is pulling in numpy 2.0 which isn't compatible with ts2vg). I couldn't get it to run locally on my machine either (but figured it was worth a shot to see if it ran in CI).

ajb5d commented 4 months ago

I added a comment and refactored the test method to use the parameterized methods.

DominiqueMakowski commented 4 months ago

amazing, thanks all!

welcome[bot] commented 4 months ago

landing Congrats on merging your first pull request! 🎉🍾 We're looking forward to your next one!