-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for dictionary-type ref_channels
in set_eeg_reference()
#12366
base: main
Are you sure you want to change the base?
Conversation
@AlexLepauvre I've come up with a short to-do list of things we may need to change to complete the PR. Feel free to modify it! My initial commit only changed the doc and slightly changed |
@@ -0,0 +1 @@ | |||
Add support for `dict` type argument ``ref_channels`` to :func:`mne._fiff.reference.set_eeg_reference`, by `Qian Chu`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what that allows users to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I don't think we should document a private function here. Can you add references to the public API instead? Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I wasn't too familiar with the private/public API and simply copied the path to the function. Does :func:mne.set_eeg_reference
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the right function! Regarding the changelog entry, can you maybe also include an example what you mean by "flexible re-referencing"? Users (not developers) should learn what nice new feature you added and how they can use it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users will rarely call this function directly though, I suppose. They will usually call the methods of Raw, Epochs, ..., objects, so I believe these should be referenced or briefly mentioned
The current implementation is based on a dictionary mapping a list to each channel. The list contains the channels to average and take as reference for the respective channel. I have also implemented an assertion to make sure that the channels in the dict correspond to the channels in the instance. We might want to implement something about bad channels. If bad channels are present in the mapping, they should probably be skipped
for more information, see https://pre-commit.ci
ref_from = pick_channels(inst.ch_names, ref_dict[ch], ordered=True) | ||
# Get indice of channel to re.reference: | ||
ref_to = pick_channels(inst.ch_names, ch, ordered=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add a warning around here if ref_from and ref_to belong to different channel types
Do you think we need a formal test for the addition (in test_reference)? |
Implement #12283
To do list:
docdict["ref_channels_set_eeg_reference"]
inmne/utils/docs.py
to includedict
set_eeg_reference()
to implementdict
based re-referencing_check_before_reference
or create new_check_before_dict_reference
_apply_reference()
or create new_apply_dict_reference()