We're currently applying reject thresholds to ECG and EOG epochs used for ICA artifact detection, but I think we shouldn't

[hoechenberger]

After ECG and EOG epochs construction, we seems to be applying the PTP rejection thresholds the user specified for "data channels":

https://github.com/mne-tools/mne-bids-pipeline/blob/7e1d9f91e3b127a13295536c495e56d133127059/mne_bids_pipeline/steps/preprocessing/_06a2_find_ica_artifacts.py#L190-L192

https://github.com/mne-tools/mne-bids-pipeline/blob/7e1d9f91e3b127a13295536c495e56d133127059/mne_bids_pipeline/steps/preprocessing/_06a2_find_ica_artifacts.py#L235-L237

I would intuitively think that we shouldn't do this, since it could effectively eliminate part of the very artifact we're trying to isolate here. But the code is there … and I suspect we put it in for a reason?

@larsoner @agramfort @SophieHerbst Any idea what to do about this?

[hoechenberger]

@larsoner Could it be that you added this during a recent refactoring? I just (super briefly) looked at the 1.5.0 code and I don't think we have this behavior there.

[larsoner]

we seems to be applying the PTP rejection thresholds the user specified for "data channels":

Depends what you mean. This code is using epochs.reject -- but this is not config.reject (what I think you mean?) which would indeed be bad but at that point in the code effectively config.ica_reject (which seems okay) because those epochs get .drop_baded with config.ica_reject not config.reject:

https://github.com/mne-tools/mne-bids-pipeline/blob/7e1d9f91e3b127a13295536c495e56d133127059/mne_bids_pipeline/steps/preprocessing/_06a1_fit_ica.py#L202

Regarding when we had it you can follow PRs for example it was here before I moved it here, then back another you can get to here, then here, and finally here which was added by @hoechenberger in https://github.com/mne-tools/mne-bids-pipeline/pull/306 for 1.0.

For 1.5 you can see it is already here

https://github.com/mne-tools/mne-bids-pipeline/blob/bae0b2ee3c6e1941522b65b3bf9355ab172896d2/mne_bids_pipeline/steps/preprocessing/_06a_run_ica.py#L437

In any case, for SSP at least we have separate reject params which makes the most sense to me

https://github.com/mne-tools/mne-bids-pipeline/blob/7e1d9f91e3b127a13295536c495e56d133127059/mne_bids_pipeline/steps/preprocessing/_06b_run_ssp.py#L98

ICA could just use these, too (maybe after a more generic variable renaming?). But maybe using ica_reject is safe enough already.

[SophieHerbst]

So epochs.reject just applies the previously selected drops thresholded with ica_reject? That sounds right to me.

[hoechenberger]

Ok, let's leave it like this for now :) Thanks for your input, @larsoner and @SophieHerbst!