Skip to content

Remove channels with potential montage points from maxwell_filter_prepare_emptyroom #13208

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mscheltienne
Copy link
Member

Following https://mne.discourse.group/t/montage-error-in-preparing-empty-room-for-maxfilter-for-simultaneous-meg-eeg/11089

In Geneva, an acquisition with combined MEG / EEG was done while preserving the EEG channels enabled. If you blindly apply maxwell_filter_prepare_emptyroom you get a failure during the get_montage(...) / set_montage(...) step. Beside the fiducial digitization, I can't think of an information from the DigMontage required by MaxWell filter to be set on the ER. Pointing in that direction, the existing test are removing EEG channels from the test files prior to running maxwell_filter_prepare_emptyroom.

This PR adds a tiny piece of documentation about it; and drops EEG (and ECOG, sEEG, DBS channels which might also not played nicely since they would contain some un-needed electrode location information) in maxwell_filter_prepare_emptyroom. It also adds a warning informing that this channel drop happens; maybe you will need to add them back at a later stage to the ER, e.g. to run ICA if you fitted it on combined MEG / EEG?

I'm not fully convinced this is the best fix, if anyone has a better proposal, please advise.

Comment on lines +165 to +179
# just in case of combine MEG/other modality, let's explicitly drop other channels
# that might have a digitization and emit a warning.
picks = [elt for elt in ("eeg", "ecog", "seeg", "dbs") if elt in raw_er_prepared]
if len(picks) != 0:
picks = _picks_to_idx(raw_er_prepared.info, picks=picks, exclude=())
idx = np.setdiff1d(np.arange(raw_er_prepared.info["nchan"]), picks)
raw_er_prepared.pick(idx, exclude=())
if len(raw_er.ch_names) != len(raw_er_prepared.ch_names):
warn(
"The empty-room recording contained EEG-like channels. These channels "
"were dropped from the empty-room recording, as they are not "
"compatible with Maxwell filtering. If you need to, add those channels "
"back after the execution of 'maxwell_filter_prepare_emptyroom' from your "
"original empty-room recording using 'raw.add_channels'."
)
Copy link
Member Author

@mscheltienne mscheltienne Apr 14, 2025

Choose a reason for hiding this comment

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

The more I think about it, the more I feel like it should simply be .pick("meg", exclude=()). I can't think of a reason to add those channels back.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about adding the EEG channels back, but regarding .pick("meg", exclude=()) there are other channel types that you could reasonably want to have around (e.g., stim, CHPI status, active shield status, etc.) and it's hard to know every end-users use case / needs, hence the use of a blocklist approach (eeg-like) here rather than an allowlist (meg) approach.

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.

I'm not fully convinced this is the best fix, if anyone has a better proposal, please advise.

I think at UW we have recorded empty room data with EEG enabled simply because we were doing simultaneous M/EEG and didn't change the acq settings (can't remember if it was an error or standard practice anymore, but I know it happened at least sometimes!).

I am inclined to add a drop_eeg=True as a new kwarg as a bugfix since I think it's what people will want most of the time. But we could allow them to set it to False and then figure out how to fix the MF stuff if it crops up again for people who want drop_eeg=False for some reason.

@mscheltienne
Copy link
Member Author

I think at UW we have recorded empty room data with EEG enabled simply because we were doing simultaneous M/EEG and didn't change the acq settings (can't remember if it was an error or standard practice anymore, but I know it happened at least sometimes!).

Exactly what happened in GVA as well.
OK, stim, cHPI, etc.. are a good point so blocklist it is. I'm not sure there is value in an additional kwarg. Either:

  • we fix the MF code to properly ignore, silently, those channels without dropping them
  • we explicitly drop them with a warning/message (as done in this PR) since they are not meaningful for MF anyway

(2) is simpler, but I can look into (1) if you prefer, starting by adding a test and fixing the prepare ER function.

@larsoner
Copy link
Member

I think the MF code should ideally not care about the presence or absence of EEG channels. For subject data they pass through unchanged so I don't see why they wouldn't for empty-room data as well. So (1) seems better/preferable

@drammock
Copy link
Member

@mscheltienne
Copy link
Member Author

Interesting that mne-tools/mne-bids-pipeline#1040 actually picked the MEG channels and removed the rest, taking the allowlist approach.

@larsoner
Copy link
Member

MNE-BIDS-Pipeline has the luxury of being a bit more opinionated than MNE-Python because MNE-BIDS-Pipeline knows how it's internally going to use its outputs downstream to a larger extent than we can safely assume on behalf of our end users in MNE-Python 🙂 But it's also possible that we shouldn't have done that there either ...

@larsoner larsoner added this to the 1.10 milestone Apr 18, 2025
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.

3 participants