-
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 |
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
Do you think we need a formal test for the addition (in test_reference)? |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
I've pushed a few commits to tidy up the implementation, and added some inline explanations below in a self-review (the commit messages should hopefully also be helpful in understanding what I did).
I didn't yet look closely at the adequacy of the tests, other than to ensure they all pass locally for me, and adapt them slightly to account for the change making the second return value be None
(see comments below for reasoning).
One thing that I think is still missing is handling of SSP projectors, which may become invalid after referencing. The existing _check_before_reference
helper has code to handle this; that should be refactored into another helper func and called from the new _check_before_dict_reference
func too.
for more information, see https://pre-commit.ci
Just pushed a fix for the SSP projectors. Do you think we should also add a test for this in |
@drammock When you have time would you mind re-reviewing? Thanks! |
@drammock @hoechenberger @larsoner @cbrnr Hi developers, how would you like us to proceed with this PR? Thanks! |
Sorry for the slow response, I should be able to look in the coming days! |
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.
Just one tiny remaining comment which I'll push directly then mark for merge-when-green, thanks in advance @qian-chu !
mne/_fiff/reference.py
Outdated
@@ -74,6 +55,26 @@ def _check_before_reference(inst, ref_from, ref_to, ch_type): | |||
# Need to call setup_proj after changing the projs: | |||
inst._projector, _ = setup_proj(inst.info, add_eeg_ref=False, activate=False) | |||
|
|||
|
|||
def _check_before_reference(inst, ref_from, ref_to, ch_type): |
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 you move this back up? Moving it below _check_ssp
will mess up git blame
without any tangible benefit AFAICT?
@larsoner Thanks! Glad to see this feature implemented :D |
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()