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

Ensure all reference channels are used during final average referencing #92

Merged
merged 2 commits into from Jun 18, 2021

Conversation

a-hurst
Copy link
Collaborator

@a-hurst a-hurst commented Jun 18, 2021

PR Description

Closes #90. This PR fixes the issue by making reference_channels a local variable within Reference.robust_reference() so that it doesn't affect the self.reference_channels variable that's used to select reference channels during Reference.perform_reference().

As an aside, is there any situation where it would make sense to only use a subset of all channels for final average reference calculation, post-interpolation? Wondering if the whole reference_channels parameter is a carry-over from MatPREP that only exists because EEGLAB doesn't have as nice a method of marking non-EEG channels as MNE. In MatPREP, channels are excluded from being sources for interpolation if they're not specified in the "reference channels" parameter, whereas PyPREP just uses all non-bad EEG channels, but I'm not sure if that would ever actually be a problem.

Merge Checklist

  • the PR has been reviewed and all comments are resolved
  • all CI checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst

@codecov-commenter
Copy link

Codecov Report

Merging #92 (9a2ea4a) into master (6abe0bc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #92   +/-   ##
=======================================
  Coverage   98.51%   98.51%           
=======================================
  Files           7        7           
  Lines         675      675           
=======================================
  Hits          665      665           
  Misses         10       10           
Impacted Files Coverage Δ
pyprep/reference.py 96.55% <100.00%> (ø)

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 6abe0bc...9a2ea4a. Read the comment docs.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Jun 18, 2021

Also, check out the difference this makes in the MATLAB difference plot for the full PREP example in the docs. That example's not even running in MATLAB-strict mode, either!

Before this PR:
Screen Shot 2021-06-17 at 9 53 18 PM

After:
Screen Shot 2021-06-17 at 9 53 03 PM

@sappelhoff sappelhoff added this to the 0.4.0 milestone Jun 18, 2021
Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

is there any situation where it would make sense to only use a subset of all channels for final average reference calculation, post-interpolation?

I don't think so 🤔 you wouldn't even be able to call it an average reference if not all channels that are part of the data are averaged with respect to all of them.

This fix seems good, thanks for catching it!

@sappelhoff sappelhoff merged commit 4614ca0 into sappelhoff:master Jun 18, 2021
@a-hurst a-hurst mentioned this pull request Jun 18, 2021
23 tasks
@a-hurst
Copy link
Collaborator Author

a-hurst commented Jun 18, 2021

I don't think so 🤔 you wouldn't even be able to call it an average reference if not all channels that are part of the data are averaged with respect to all of them.

That's what I thought. Wondering if that part of the default API (i.e., needing to specify "referenced" and "rereferenced" channels manually in the params dict instead of just defaulting to all EEG channels) might be worth changing before PyPREP officially leaves its "experimental" stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyPREP excludes interpolated NaN/flat/low-SNR channels from average referencing
3 participants