NeuroKit
NeuroKit copied to clipboard
[Feature] Set interpolation rate of interbeat intervals
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:
- There is a new argument
interpolation_rate
instead of always setting thesampling_rate
of the raw heartbeat data as the interpolation rate of the interbeat intervals. - The default is 1000 Hz (up for discussion?).
- 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
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 :)
It seems like this is breaking several tests, we need to evaluate that and update the tests if need be
I'm half-convinced ab out the 4 Hz by default, it just seems really low to get frequency decomposition :/
@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)."
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
(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.
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 :)
It seems like this broke hrv_rsa()
Codecov Report
Merging #680 (b5c87ac) into dev (ec5d9e3) will increase coverage by
0.00%
. The diff coverage is88.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.
@DominiqueMakowski sorry for not fixing that bug earlier, I think it's good now!
looks good, thanks a lot Dani