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 min, max, 80th & 20th percentiles as HRV features #627

Merged

Conversation

danibene
Copy link
Collaborator

Add the minimum, maximum, 20th percentile, and 80th percentile NN as features in the hrv_time function.

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

@welcome
Copy link

welcome bot commented Mar 26, 2022

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

@@ -50,8 +50,12 @@ def hrv_time(peaks, sampling_rate=1000, show=False, **kwargs):
- **HCVNN**: The median absolute deviation of the RR intervals (MadNN) divided by the median
of the absolute differences of their successive differences (MedianNN).
- **IQRNN**: The interquartile range (IQR) of the RR intervals.
- **Prc20NN**: The 20th percentile of the RR intervals.
Copy link
Member

Choose a reason for hiding this comment

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

@danibene Great, thanks, very clean PR :)

It's mostly good, but could you perhaps add one or two references in which they use these indices? You can add the full ref in the Reference section and the short form (refauthor, 1998) after the index (in the future we could probably add that to all of the indices but that will be WIP)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you :)

I added two references for each but please feel free to add/remove/edit.

@DominiqueMakowski
Copy link
Member

Also, consider adding yourself to the contributor list if you want

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022

Codecov Report

Merging #627 (8619557) into dev (a30f76e) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #627      +/-   ##
==========================================
+ Coverage   82.54%   82.62%   +0.07%     
==========================================
  Files         235      235              
  Lines       11276    11289      +13     
==========================================
+ Hits         9308     9327      +19     
+ Misses       1968     1962       -6     
Impacted Files Coverage Δ
neurokit2/hrv/hrv_time.py 99.05% <100.00%> (+0.97%) ⬆️
neurokit2/eeg/eeg_rereference.py 83.33% <0.00%> (-3.77%) ⬇️
neurokit2/ecg/__init__.py 100.00% <0.00%> (ø)
neurokit2/microstates/microstates_plot.py 84.84% <0.00%> (ø)
neurokit2/signal/signal_distort.py 96.47% <0.00%> (+0.04%) ⬆️
neurokit2/ecg/ecg_simulate.py 98.43% <0.00%> (+0.17%) ⬆️
neurokit2/hrv/hrv_nonlinear.py 97.22% <0.00%> (+0.43%) ⬆️
neurokit2/rsp/rsp_simulate.py 99.10% <0.00%> (+4.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a30f76e...8619557. Read the comment docs.

@DominiqueMakowski
Copy link
Member

Looks great, thanks again @danibene !! will merge as soon as the checks pass

@DominiqueMakowski DominiqueMakowski merged commit 57a42ff into neuropsychology:dev Mar 29, 2022
@welcome
Copy link

welcome bot commented Mar 29, 2022

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

@danibene danibene deleted the feature/min_max_perc_hrv branch August 18, 2022 03:53
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.

3 participants