mne-tools / mne-bids-pipeline

Automatically process entire electrophysiological datasets using MNE-Python.
https://mne.tools/mne-bids-pipeline/
BSD 3-Clause "New" or "Revised" License
137 stars 65 forks source link

Use cleaned epochs for sensor-space decoding, time-frequency analysis, and covariance calculation #796

Closed hoechenberger closed 10 months ago

hoechenberger commented 10 months ago

We accidentally used the non-cleaned epochs for classification. This issue probably snuck in during the recent set of refactorings.

Before merging …

hoechenberger commented 10 months ago

I suppose the correct fix would be:

    processing = "clean" if cfg.spatial_filter is not None else None

We do that in _06_make_cov.py

hoechenberger commented 10 months ago

I suppose the correct fix would be:

    processing = "clean" if cfg.spatial_filter is not None else None

We do that in _06_make_cov.py

Actually I believe this is just a leftover from a time where we wouldn't create "proc-clean" epochs if no spatial filter was run. Now, we always create those "proc-clean" epochs, even if spatial filtering (ICA, SSP) is skipped. It happens in the step that applied PTP rejection thresholds.

So I've now changed all places where we use epochs after pre-processing to always (and only) use "proc-clean" epochs.

hoechenberger commented 10 months ago

CI errors indicate that I'm probably overlooking something

larsoner commented 10 months ago

The ds001810 failure seems real, like somehow an extra condition is sneaking into the classification somehow now. @hoechenberger do you have time to check locally if that's the case?

hoechenberger commented 10 months ago

@hoechenberger do you have time to check locally if that's the case?

Unfortunately not, I've been trying to download that dataset for the past hour but it's just hopeless with our firewall here.

hoechenberger commented 10 months ago

@larsoner Btw what I suspect is happening is that we simply end up with too few clean epochs, hence CV won't work

Maybe we can set the number of splits to 2 or even 1... that should fix the issue

larsoner commented 10 months ago

Ahh makes sense. So use max(5, n_epochs_per_cond_min) or whatever. I'll look and push a commit, including reverting my always: conftest change that hopefully will no longer be necessary.

hoechenberger commented 10 months ago

So use max(5, n_epochs_per_cond_min) or whatever.

I actually do like the fact that we're currently failing hard

We should adjust the test case for that specific dataset instead of auto-adjusting the number of splits

hoechenberger commented 10 months ago

@larsoner I just tried, I cannot reproduce this locally :(

hoechenberger commented 10 months ago

@larsoner It seems the issue occurs during the cathodalpost session:

│18:59:14│ ⏳️ sub-01 ses-cathodalpost Contrasting conditions: 61450 – 61511
FAILED           [100%]
hoechenberger commented 10 months ago
import mne
epo = mne.read_epochs("sub-01_ses-cathodalpost_task-attentionalblink_proc-clean_epo.fif")
epo

gives:

<EpochsFIF |  25 events (all good), -0.199219 – 0.5 s, baseline -0.199219 – 0 s, ~5.1 MB, data loaded, with metadata,
 '61450': 22
 '61511': 3>
agramfort commented 10 months ago

@hoechenberger did you check on a few datasets if it boosts the decoding scores? as decoding learns discriminative features it's usually pretty good at filtering out artifacts.

hoechenberger commented 10 months ago

@agramfort No, I did not systematically check this. Are you saying it may have been a conscious decision to use uncleaned epochs here? Because to me, it looked unintentional (i.e., a bug)

Especially since we also used the uncleaned epochs for covariance calculation

agramfort commented 10 months ago

Yes I think it was intentional although not well motivated and documented. Just my prior experience

hoechenberger commented 10 months ago

Maybe we should add a setting, decoding_use_clean_epochs: bool

hoechenberger commented 10 months ago

The thing is that we always use cleaned epochs to create the Evokeds, and source reconstruction also operates on these clean Evokeds. So we're introducing some sort of an inconsistency if decoding shall operate on un-cleaned epochs.

One could argue that if one doesn't want to decode cleaned epochs, one could just disable SSP/ICA and the PTP rejection step…

agramfort commented 10 months ago

I would just check on a couple of datasets to see what it changes

hoechenberger commented 10 months ago

@agramfort Now this is anecdotical if you will, but I'm currently working with EEG data and one recording was particularly contaminated with a certain artifact that would spread across many sensors. Neither full-epochs nor time-by-time decoding could find a difference between two experimental conditions, and I was about to exclude this participant from analysis. I then cleaned the data (autoreject local -> ICA -> autoreject local) and now full-epochs decoding scores very well above chance (~0.9 ROC AUC) and several time periods are above chance level (~0.6 ROC AUC) in time-by-time decoding. For several other participants, decoding performance was improved by this approach too.

That is to say, I did find the expected effect even when using the un-cleaned data, but it's now larger and more obvious

larsoner commented 10 months ago

My sense is that things like SSP and ICA probably won't matter but that peak-to-peak rejection could help, depending on the nature of the artifacts. That sounds in line with what you're seeing @hoechenberger . In this case it makes sense for us to use the "clean" epochs for decoding. At some point we could make it an option which to use (but maybe assume YAGNI for now?)

agramfort commented 10 months ago

fine with me !

thanks for the details

Message ID: @.***>