NeuroKit icon indicating copy to clipboard operation
NeuroKit copied to clipboard

[Fix] Calculation of number of NaNs in signal_fixpeaks()

Open danibene opened this issue 1 year ago • 4 comments

Description

This PR aims to improve the previous PR: https://github.com/neuropsychology/NeuroKit/pull/651 See also the related issue: https://github.com/neuropsychology/NeuroKit/issues/650

Proposed Changes

I changed the neurokit method of the signal_fixpeaks() function so that the number of NaNs inserted is computed as follows:

  1. Rounding rather than taking the floor when getting an integer value for the number of NaNs (calculated by dividing large gaps by the mean interval)
  2. Calculating the mean interval based on the original interval values and not the standardized interval values in the case that relative_interval_max is given.

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. I got the following error, but I'm just going to ignore it if that's okay, since I don't think I changed anything that should cause this? astroid.exceptions.TooManyLevelsError: Relative import with too many levels (2) for module 'signal_fixpeaks' Could also be the fact that I have a file called signal_fixpeaks.py in a different folder.
  • [ ] I have added the newly added features to News.rst (if applicable)

danibene avatar Jul 20 '22 01:07 danibene

Minor style issue https://github.com/neuropsychology/NeuroKit/runs/7420854905?check_suite_focus=true#step:8:23 to fix before we can merge!

DominiqueMakowski avatar Jul 20 '22 03:07 DominiqueMakowski

Minor style issue https://github.com/neuropsychology/NeuroKit/runs/7420854905?check_suite_focus=true#step:8:23 to fix before we can merge!

@DominiqueMakowski if I understood correctly it's an issue with signal_detrend()? Should I modify that file?

danibene avatar Jul 20 '22 04:07 danibene

oops yeah my bad I introduced that just now 🙈 I'll fix it on dev then

DominiqueMakowski avatar Jul 20 '22 04:07 DominiqueMakowski

Codecov Report

Merging #679 (8cdac4b) into dev (597594b) will increase coverage by 0.04%. The diff coverage is 78.26%.

@@            Coverage Diff             @@
##              dev     #679      +/-   ##
==========================================
+ Coverage   52.47%   52.52%   +0.04%     
==========================================
  Files         277      277              
  Lines       12508    12502       -6     
==========================================
+ Hits         6564     6567       +3     
+ Misses       5944     5935       -9     
Impacted Files Coverage Δ
neurokit2/signal/signal_fixpeaks.py 73.00% <78.26%> (+0.14%) :arrow_up:
neurokit2/eda/eda_eventrelated.py 83.67% <0.00%> (+2.04%) :arrow_up:
neurokit2/rsp/rsp_simulate.py 99.10% <0.00%> (+5.35%) :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 Jul 20 '22 04:07 codecov-commenter

@danibene apologies for the delay - it's been a busy month! I'm gonna merge this once it passes the checks :)

DominiqueMakowski avatar Aug 18 '22 03:08 DominiqueMakowski

@DominiqueMakowski nice, thanks :)

danibene avatar Aug 18 '22 03:08 danibene