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

In sensor/apply_ica, we should not apply the ICA rejection threshold to the data before rejecting components #805

Closed
hoechenberger opened this issue Nov 1, 2023 · 1 comment · Fixed by #806
Assignees
Labels
bug Something isn't working

Comments

@hoechenberger
Copy link
Member

hoechenberger commented Nov 1, 2023

We have a setting ica_reject to remove bad epochs before fitting ICA. The epochs we feed into ICA are separate from the epochs we want to clean: they're high-pass filtered and they have drop_bad(reject=ica_reject) applied. This ensures ICA can be fit on epochs without flux / voltage spikes etc

Now in the apply_ica step, when we load the "actual" epochs we wish to clean, instead of simply applying ICA, we first call drop_bad(reject=ica_reject) too. In my opinion this is a bug, as normally we should be assuming that applying ICA may at least remove some of those high-amplitude artifacts. But if we reject those epochs before cleaning them via ICA, they're gone forever.

epochs = mne.read_epochs(in_files.pop("epochs"), preload=True)
ica_reject = _get_reject(
subject=subject,
session=session,
reject=cfg.ica_reject,
ch_types=cfg.ch_types,
param="ica_reject",
)
epochs.drop_bad(ica_reject)
# Now actually reject the components.
msg = f'Rejecting ICs: {", ".join([str(ic) for ic in ica.exclude])}'
logger.info(**gen_log_kwargs(message=msg))
epochs_cleaned = ica.apply(epochs.copy()) # Copy b/c works in-place!

I think we should remove the drop_bad() call here. Any remaining large-amplitude artifacts after ICA cleaning will be caught in the following step, preprocessing/ptp_reject.

In an EEG dataset I'm currently working with, applying the PTP rejection threshold before applying ICA leads to a loss of many epochs that could have been saved through ICA

cc @agramfort

@hoechenberger hoechenberger added the bug Something isn't working label Nov 1, 2023
@hoechenberger hoechenberger changed the title In sensor/apply_ica, we should not apply the ICA rejection threshold to the data before rejecting component In sensor/apply_ica, we should not apply the ICA rejection threshold to the data before rejecting components Nov 1, 2023
@larsoner
Copy link
Member

larsoner commented Nov 1, 2023

Agreed @hoechenberger feel free to fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants