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

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

Merged

Conversation

ajb5d
Copy link
Contributor

@ajb5d ajb5d commented Jul 10, 2024

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 :)

  • 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)

Copy link

welcome bot commented Jul 10, 2024

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).

Copy link
Collaborator

@danibene danibene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ajb5d , thank you for this contribution! I would suggest also adding a test of the case when the array should be empty if possible. I'm happy to help you do this if you need any support.

neurokit2/ecg/ecg_findpeaks.py Outdated Show resolved Hide resolved
@DerAndereJohannes
Copy link
Contributor

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
Copy link
Contributor Author

ajb5d commented Jul 12, 2024

@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):

  • I had initially added a check that they return no peaks when presented a flat array but a couple of the methods do find peaks in an empty signal. That feels out of scope.
  • The promac method calls other methods and returns errors when no peaks are found by those methods. I think the root cause is _ecg_findpeaks_promac_addconvolve line 223: mask[peaks] = 1 which fails when peaks is empty. I wasn't certain of the best way to handle this nested error so I'm leaving it untouched.

@ajb5d ajb5d requested a review from danibene July 12, 2024 13:47
@ajb5d
Copy link
Contributor Author

ajb5d commented Jul 12, 2024

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).

Copy link
Collaborator

@danibene danibene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes! If you are unable to get the test working for the vgraph method, I think it is fine to merge without for now, as long as it is documented as a known issue.

@ajb5d
Copy link
Contributor Author

ajb5d commented Jul 12, 2024

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

@@ -23,6 +25,17 @@ def _read_csv_column(csv_name, column):
csv_data = pd.read_csv(csv_path, header=None)
return csv_data[column].to_numpy()

#vgraph is not included because it currently causes CI to fail (issue 1007)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#vgraph is not included because it currently causes CI to fail (issue 1007)
#vgraph is not included because it currently causes CI to fail (issue #1009)

Copy link
Collaborator

@danibene danibene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@DominiqueMakowski
Copy link
Member

amazing, thanks all!

@DominiqueMakowski DominiqueMakowski changed the title Return an empty array rather than throwing an exception if no QRS dat… [Fix] Return an empty array rather than throwing an exception if no QRS dat… Jul 12, 2024
@DominiqueMakowski DominiqueMakowski merged commit 1151582 into neuropsychology:dev Jul 12, 2024
8 checks passed
Copy link

welcome bot commented Jul 12, 2024

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

@ajb5d ajb5d mentioned this pull request Jul 14, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants