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

[Feature] Add Fast Visibility Graph Based ECG R-peak Detector #939

Merged
merged 8 commits into from
Dec 21, 2023

Conversation

JonasEmrich
Copy link
Contributor

@JonasEmrich JonasEmrich commented Dec 12, 2023

Copy link

welcome bot commented Dec 12, 2023

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

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

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

Comparison is base (87c8c60) 54.82% compared to head (72e7601) 54.60%.

Files Patch % Lines
neurokit2/ecg/ecg_findpeaks.py 14.91% 97 Missing ⚠️
neurokit2/ecg/ecg_clean.py 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #939      +/-   ##
==========================================
- Coverage   54.82%   54.60%   -0.22%     
==========================================
  Files         304      304              
  Lines       14269    14333      +64     
==========================================
+ Hits         7823     7827       +4     
- Misses       6446     6506      +60     

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

@JonasEmrich
Copy link
Contributor Author

Now, I should have adapted the package at all the required places. Together with the authors of the koka2022 method, we decided to remove it in favor of this more advanced method.

I tested the implementation briefly on two databases, and it appears that it procures results comparable to the original FastNVG algorithm (https://github.com/JonasEmrich/vg-beat-detectors)

@JonasEmrich JonasEmrich marked this pull request as ready for review December 14, 2023 11:25
@DominiqueMakowski
Copy link
Member

Amazing job! Thanks

Together with the authors of the koka2022 method, we decided to remove it in favor of this more advanced method.

In that case, could we leave the "koka2022" input to then run your udpated method, throwing a warning that "The 'koka2022' method has been replaced by XX. Please replace method='koka2022' by ..." (so that we don't break potential user code just yet)

@JonasEmrich
Copy link
Contributor Author

JonasEmrich commented Dec 15, 2023

In that case, could we leave the "koka2022" input to then run your udpated method, throwing a warning that "The 'koka2022' method has been replaced by XX. Please replace method='koka2022' by ..." (so that we don't break potential user code just yet)

Thanks for pointing this out! Now, I added a warning in ecg_clean() and ecg_findpeaks().

@DominiqueMakowski
Copy link
Member

I think we can just add it in ecg_findpeaks() because it's there that the switch happens

@JonasEmrich
Copy link
Contributor Author

I think we can just add it in ecg_findpeaks() because it's there that the switch happens

Although the emrich2023 method uses the same cleaning as koka2022 (points to the same function), I think that perhaps users also should use emrich2023 here instead of koka2022. It's just the naming but makes it more consistent, since the koka2022 method is deprecated/removed. Or what are your thoughts?

@DominiqueMakowski
Copy link
Member

It's just the naming but makes it more consistent, since the koka2022 method is deprecated/removed

makes sense!

@DominiqueMakowski
Copy link
Member

@JonasEmrich is it good for me to merge?

@JonasEmrich
Copy link
Contributor Author

@JonasEmrich is it good for me to merge?

@DominiqueMakowski yes, from my side it is ready to merge

@DominiqueMakowski
Copy link
Member

Amazing, thanks again for all the work :)

@DominiqueMakowski DominiqueMakowski merged commit 51b48bd into neuropsychology:dev Dec 21, 2023
8 checks passed
Copy link

welcome bot commented Dec 21, 2023

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

@NikkiNgombe
Copy link

Hello @JonasEmrich and @DominiqueMakowski ,
I'm working with some ECG and wanted to use "emrich2023" for the peak detection. However, I get the following error:

ValueError: NeuroKit error: ecg_findpeaks(): 'emrich2023' not implemented.

After installing ts2vg, I was able to use koka2022, but I am still unable to run ecg_findpeaks() using the Emrich 2023 method. Is this emrich2023 method actually implemented in NeuroKit2? When I checked the source code for ecg_findpeaks(), I also noticed, that emrich2023 wasn't listed in the available methods.
Screenshot 2024-07-02 at 20 30 42

Any help would be greatly appreciated, thank you!
Nikki

@DominiqueMakowski
Copy link
Member

I can see the lines in the current version:

elif method in ["vg", "vgraph", "fastnvg", "emrich", "emrich2023"]:
clean = _ecg_clean_vgraph(ecg_signal, sampling_rate)

Do you have the latest version of NK installed?

@NikkiNgombe
Copy link

Thank you for the quick response! I was using version 0.2.7. I have now updated to version 0.2.9 and can now use the emrich2023 option. Thank you so much! :)

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