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] filter signals with missing data #839

Merged

Conversation

danibene
Copy link
Collaborator

@danibene danibene commented Jun 7, 2023

Description

As a step towards better handling of missing data (see #838), this PR aims to allow for filtering signals even if they contain NaNs.

Proposed Changes

  1. I changed the signal_filter() function so that NaNs are interpolated before filtering and then added back after filtering.
  2. I added test_signal_filter_with_missing() to test that the signal containing NaNs is filtered.

Checklist

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).

@danibene
Copy link
Collaborator Author

danibene commented Jun 7, 2023

I wonder, should we have a warning at this stage if missing samples are detected (or a certain proportion of missing samples)? Or only have warnings in specific feature extraction functions?

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Patch coverage: 86.66% and project coverage change: +0.06 🎉

Comparison is base (5116fb4) 53.88% compared to head (2713a37) 53.95%.

❗ Current head 2713a37 differs from pull request most recent head 9bfec57. Consider uploading reports for the commit 9bfec57 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #839      +/-   ##
==========================================
+ Coverage   53.88%   53.95%   +0.06%     
==========================================
  Files         295      295              
  Lines       13811    13820       +9     
==========================================
+ Hits         7442     7456      +14     
+ Misses       6369     6364       -5     
Impacted Files Coverage Δ
neurokit2/signal/signal_filter.py 70.90% <86.66%> (+2.28%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DominiqueMakowski
Copy link
Member

I wonder, should we have a warning at this stage if missing samples are detected (or a certain proportion of missing samples)? Or only have warnings in specific feature extraction functions?

Mmh good question... any preference?

@danibene
Copy link
Collaborator Author

I wonder, should we have a warning at this stage if missing samples are detected (or a certain proportion of missing samples)? Or only have warnings in specific feature extraction functions?

Mmh good question... any preference?

Not really, I noticed that there is already a warning in ecg_clean():

n_missing = np.sum(np.isnan(ecg_signal))
if n_missing > 0:
warn(
"There are " + str(n_missing) + " missing data points in your signal."
" Filling missing values by using the forward filling method.",
category=NeuroKitWarning,
)
ecg_signal = _ecg_clean_missing(ecg_signal)

So I guess to avoid being repetitive we can leave it out for now.

@DominiqueMakowski
Copy link
Member

Sounds good, feel free to merge

@danibene danibene merged commit 774458f into neuropsychology:dev Jun 13, 2023
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