Skip to content

Add support for dictionary-type ref_channels in set_eeg_reference() #12366

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

Merged
merged 72 commits into from
Sep 24, 2024

Conversation

qian-chu
Copy link
Contributor

@qian-chu qian-chu commented Jan 16, 2024

Implement #12283

To do list:

  • Change docdict["ref_channels_set_eeg_reference"] in mne/utils/docs.py to include dict
  • Change set_eeg_reference() to implement dict based re-referencing
    • Modify _check_before_reference or create new _check_before_dict_reference
    • Modify _apply_reference() or create new _apply_dict_reference()

@qian-chu
Copy link
Contributor Author

@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 reference. Everything is subject to change as you see fit :)

qian-chu and others added 4 commits January 17, 2024 17:53
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
@qian-chu
Copy link
Contributor Author

qian-chu commented May 3, 2024

Do you think we need a formal test for the addition (in test_reference)?

@larsoner larsoner modified the milestones: 1.8, 1.9 Aug 7, 2024
@qian-chu
Copy link
Contributor Author

@drammock When you have time would you mind re-reviewing? Thanks!

@qian-chu
Copy link
Contributor Author

@drammock @hoechenberger @larsoner @cbrnr Hi developers, how would you like us to proceed with this PR? Thanks!

@larsoner
Copy link
Member

Sorry for the slow response, I should be able to look in the coming days!

Copy link
Member

@larsoner larsoner left a 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 !

@@ -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):
Copy link
Member

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 larsoner enabled auto-merge (squash) September 20, 2024 17:32
@larsoner larsoner disabled auto-merge September 20, 2024 17:32
@larsoner larsoner enabled auto-merge (squash) September 20, 2024 17:33
@larsoner larsoner dismissed drammock’s stale review September 20, 2024 17:33

Comments addressed

@qian-chu
Copy link
Contributor Author

@larsoner Thanks! Glad to see this feature implemented :D

@qian-chu
Copy link
Contributor Author

Seems Ubuntu pip test is having some difficulty configuring its dependency. Hope it's irrelevant to the PR

@larsoner
Copy link
Member

Yes should be okay so I restarted the test -- I'll keep and eye on it!

@larsoner larsoner disabled auto-merge September 24, 2024 14:47
@larsoner larsoner merged commit d917be1 into mne-tools:main Sep 24, 2024
26 of 27 checks passed
@larsoner
Copy link
Member

Thanks @qian-chu !

@qian-chu qian-chu deleted the dict_ref branch September 24, 2024 14:56
larsoner added a commit to mscheltienne/mne-python that referenced this pull request Sep 24, 2024
* upstream/main:
  Add support for dictionary-type `ref_channels` in `set_eeg_reference()` (mne-tools#12366)
  [MRG] Remove impedances in ant reader in favor of an example using antio (mne-tools#12868)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12869)
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.

6 participants