NeuroKit icon indicating copy to clipboard operation
NeuroKit copied to clipboard

[Feature] Set interpolation rate of interbeat intervals

Open danibene opened this issue 1 year ago • 1 comments

Description

This PR aims to enable custom interpolation rates of interbeat intervals. See https://github.com/neuropsychology/NeuroKit/issues/671

Proposed Changes

I changed the _hrv_get_rri() function so that:

  1. There is a new argument interpolation_rate instead of always setting the sampling_rate of the raw heartbeat data as the interpolation rate of the interbeat intervals.
  2. The default is 1000 Hz (up for discussion?).
  3. There is no more minimum requirement of 10 Hz (up for discussion?).

Checklist

  • [x] I have read the CONTRIBUTING file.
  • [x] My PR is targeted at the dev branch (and not towards the master branch).
  • [x] I ran the CODE CHECKS on the files I added or modified and fixed the errors. Got the same error as in https://github.com/neuropsychology/NeuroKit/pull/679 that I will continue to ignore if that's okay: astroid.exceptions.TooManyLevelsError: Relative import with too many levels (2) for module 'hrv_utils'
  • [ ] I have added the newly added features to News.rst (if applicable) I am still confused about where these should be listed

danibene avatar Jul 20 '22 17:07 danibene

Regarding the failing checks (sorry, didn't notice until I checked my email):

For README_examples: I have changed the line causing the error, interpolated[int(x_values[-1]) :] = interpolated[int(x_values[-1])], in https://github.com/neuropsychology/NeuroKit/pull/666 - maybe it makes sense to decide on whether those changes will be merged before addressing the current PR?

For tests_hrv, I'm having trouble interpreting the output: tests/tests_hrv.py::test_hrv_frequency ERROR: InvocationError for command /home/runner/work/NeuroKit/NeuroKit/.tox/py/bin/pytest tests neurokit2/bio neurokit2/complexity neurokit2/data neurokit2/ecg neurokit2/eda neurokit2/eeg neurokit2/emg neurokit2/eog neurokit2/epochs neurokit2/events neurokit2/hrv neurokit2/microstates neurokit2/misc neurokit2/ppg neurokit2/rsp neurokit2/signal neurokit2/stats -v (exited with code -9 (SIGKILL)) (exited with code -9) Would it be possible for someone to help me with this?

Thanks :)

danibene avatar Jul 20 '22 21:07 danibene

It seems like this is breaking several tests, we need to evaluate that and update the tests if need be

DominiqueMakowski avatar Aug 18 '22 07:08 DominiqueMakowski

I'm half-convinced ab out the 4 Hz by default, it just seems really low to get frequency decomposition :/

DominiqueMakowski avatar Aug 18 '22 07:08 DominiqueMakowski

@DominiqueMakowski

I'm half-convinced ab out the 4 Hz by default, it just seems really low to get frequency decomposition :/

How about a default of 7 or 10 Hz?

From https://pubmed.ncbi.nlm.nih.gov/15825865/

"Most of the papers in the field of HRV report on the use of resampling rates between 2 and 4 Hz (although 1 and 10 Hz have also been used) [2], [3]. Choosing a resampling rate of 7 Hz for the RR tachogram has the advantage that HR <= 210 BPM can be resolved (see Section III-A2 on the average Nyquist criterion)."

danibene avatar Aug 18 '22 07:08 danibene

That sounds better, though it's still unclear to me why not resample at higher rates, I mean the computational overhead is quite and higher rates could accommodate users that work with animals (that can have super high bpms)

I mean aside from "it's not what kubios does", it's not clear to me the disadvantages of resampling at 100+ Hz... do you know of any? (I'm sorry for blocking here but I wanna make sure we make the best decisions for our defaults ^^)

DominiqueMakowski avatar Aug 24 '22 11:08 DominiqueMakowski

@DominiqueMakowski

(I'm sorry for blocking here but I wanna make sure we make the best decisions for our defaults ^^)

No worries, I appreciate the due diligence :)

I mean aside from "it's not what kubios does", it's not clear to me the disadvantages of resampling at 100+ Hz... do you know of any?

Not really, I mainly thought for reproducibility's sake it's better to keep it within the range that is used in the literature, as well in other software beyond Kubios e.g.

  • 4 Hz: https://github.com/PGomes92/pyhrv/blob/master/pyhrv/frequency_domain.py#L161
  • 4 Hz: https://github.com/Aura-healthcare/hrv-analysis/blob/master/hrvanalysis/extract_features.py#L218-L220

But I also understand if you'd rather not change it from the current default, and with interpolation_rate as a separate argument, the user will be able to specify anyway.

danibene avatar Aug 24 '22 15:08 danibene

I am pretty sure these two packages you mentioned have 4 Hz just because of kubios though 😁

Okay, in that case let's put it at 100, which should be enough to cover animal-fast heart rates, and at the same time leads to 10 times shorter signals than 1000 Hz for the analyses (so we gain in efficiency). We can add in the docstring that in order to replicate the results from other software such as Kubios, one can set the argument to 4.

And in a comment right above where we resample, we can summarize our thoughts: We thought to use 4 by default to match Kubios, but it seems too low (especially for some applications with fast heart rates), hence we have put it to 100 by default, but if anybody has any better idea, please do let us know. And we can add a link to this PR :)

DominiqueMakowski avatar Aug 25 '22 00:08 DominiqueMakowski

It seems like this broke hrv_rsa()

DominiqueMakowski avatar Aug 25 '22 03:08 DominiqueMakowski

Codecov Report

Merging #680 (b5c87ac) into dev (ec5d9e3) will increase coverage by 0.00%. The diff coverage is 88.88%.

@@           Coverage Diff           @@
##              dev     #680   +/-   ##
=======================================
  Coverage   52.75%   52.76%           
=======================================
  Files         277      277           
  Lines       12611    12615    +4     
=======================================
+ Hits         6653     6656    +3     
- Misses       5958     5959    +1     
Impacted Files Coverage Δ
neurokit2/hrv/hrv_frequency.py 61.70% <ø> (ø)
neurokit2/hrv/hrv_utils.py 62.31% <88.88%> (+0.78%) :arrow_up:

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

codecov-commenter avatar Aug 25 '22 04:08 codecov-commenter

@DominiqueMakowski sorry for not fixing that bug earlier, I think it's good now!

danibene avatar Aug 25 '22 05:08 danibene

looks good, thanks a lot Dani

DominiqueMakowski avatar Aug 25 '22 05:08 DominiqueMakowski