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] ECG delineator peak index #731

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

danibene
Copy link
Collaborator

Description

This PR aims to fix an error in ecg_delineate() where invalid indices were returned. See also #729

Proposed Changes

I changed the ecg_delineate() function so that within _ecg_delineator_peak(), all indices of fiducial points are checked to ensure that they are not larger than the number of samples in the ECG signal - 1

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 only actively corrected sections of the code that I modified here)

@danibene danibene changed the base branch from master to dev October 13, 2022 15:20
@codeclimate
Copy link

codeclimate bot commented Oct 13, 2022

Code Climate has analyzed commit 8331d3c and detected 0 issues on this pull request.

View more on Code Climate.

@danibene
Copy link
Collaborator Author

Also should we add https://github.com/neuropsychology/NeuroKit/files/9758396/debug.csv to https://github.com/neuropsychology/NeuroKit/tree/master/tests/ecg_data & use the code that led to the error as a test for ecg_delineate()? I just tested it by installing my branch:

Pip install: https://gist.github.com/danibene/8b5df2192b5a063d10e59972d77556d6
Installing my branch: https://gist.github.com/danibene/6cb93f7c2654698b91f0f3a5de2e8166

but maybe it'd be good to have it as a proper test?

@codecov-commenter
Copy link

Codecov Report

Base: 53.33% // Head: 53.32% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (8331d3c) compared to base (c88602e).
Patch coverage: 93.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #731      +/-   ##
==========================================
- Coverage   53.33%   53.32%   -0.01%     
==========================================
  Files         283      283              
  Lines       12844    12845       +1     
==========================================
  Hits         6850     6850              
- Misses       5994     5995       +1     
Impacted Files Coverage Δ
neurokit2/ecg/ecg_delineate.py 79.36% <93.33%> (-0.12%) ⬇️
neurokit2/signal/signal_formatpeaks.py 81.25% <100.00%> (ø)
neurokit2/signal/signal_fixpeaks.py 72.90% <0.00%> (-0.11%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@DominiqueMakowski DominiqueMakowski left a comment

Choose a reason for hiding this comment

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

Ah nice, thanks for digging into the delineator code, it's a tough one :)

@danibene danibene linked an issue Oct 13, 2022 that may be closed by this pull request
@danibene danibene merged commit c0aad5f into neuropsychology:dev Oct 13, 2022
@danibene danibene deleted the fix/ecg_delineator_peak_index branch October 13, 2022 23:54
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.

"[N] not in index" exception raised by ecg_delineate
3 participants