Add the code from PR "Add support for MNE-ICALabel #812" #1018

jschepers · 2024-11-07T10:38:30Z

Changes

Added the code from Add support for MNE-ICALabel #812 by @hoechenberger
Comment: In the newer version of mne-bids-pipeline the ICA is divided into two scripts, therefore I had to divide the code into these scripts.
@behinger added two config variables ica_use_ecg_detection and ica_use_eog_detection to control whether MNE eog/ecg detection should be used on the ICs
We implemented that the user can specify in the config file (using the variable icalabel_include) which labels/ICs to include. Default is ["brain","other"].
~~Comment: We should add tests for this e.g. No value given -> Default should be used, Empty list or wrong labels -> Error~~ No need to test that pydantic does this -- tested locally quickly and seems to do the right thing
Added plots of the excluded ICs to the report together with their labels and the probability of the labels.
Comment: The plotting needs improvement e.g. combine all single plots in one plot
Changelog has been updated (docs/source/dev.md.inc)

Closes #812

…input whether to do eog or ecg detection on ICs

…ic_label

…input whether to do eog or ecg detection on ICs

welcome · 2024-11-07T10:38:32Z

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

for more information, see https://pre-commit.ci

larsoner

Looks like a good start! Just a bunch of minor stuff here. Let me know if you want me to push some commits to make CIs happy

mne_bids_pipeline/_config.py

larsoner · 2024-11-07T17:50:58Z

mne_bids_pipeline/_config.py

+            "other",
+        ]
+    ],
+    Len(1, 7),


If we are really going for broke on this one, we should use the unique pydantic workaround

pydantic/pydantic-core#820 (comment)

If you don't want to implement it here maybe add it as a # TODO: comment in the ICA preprocessing scripts somewhere? (Don't add it in this file as it would make it messier.)

I dont think I can really follow you there. Maybe something got lost, but what does "going for broke" mean?

@jschepers I dont actually see where we really select components to be excluded, but in their tutorial it is also quite confusing. I remember we looked at it, but it was some time ago. E.g. in eeglab you specify "remove muscle if probability >80%" and similar. How is this done here, do you remember? Else I will try to give this another spin in debug mode

I dont think I can really follow you there. Maybe something got lost, but what does "going for broke" mean?

Sorry it's just an idiom -- in this case I mean if you want to put forth potentially a lot of effort to try to come up with a more complete/cool solution, you could. What we want here in not just a list with length between 1 and 7 with elements from a set of possible choices, but rather that they are unique elements (i.e., a user shouldn't put in ["eye blink", "eye blink"]). But what you have here is already good enough!

Okay I finally came back to this! Things seem to be working at my end.

FYI some of the components have a kind of low probability... looked like in testing at one point one of the things labeled as blink had a probability of like 0.35 or so. So not sure if you want to add a threshold as suggested by @behinger above. But in the meantime I think this is working as intended, @behinger @jschepers feel free to try it and look etc!

mne_bids_pipeline/steps/preprocessing/_06a1_fit_ica.py

mne_bids_pipeline/_config.py

pyproject.toml

behinger · 2024-11-12T16:49:01Z

thanks for the quick feedback! We will still need some time to work on this, too many parallel things going on.

Because we mainly develop in Julia, not everything is clear to us. I will ask some clarifications on the workflow.

Just foreshadowing two PRs we want to submit:

eyelink synchronisation: we understand it is a bit specific, but it is implemented in MNE, so we added a new step to support it (80% ready for PR)
zapline: this will be added to the filter thing as a quasi-replacement for 50/60Hz notch filtering, based on meegkit (only just started with the planning of that one).

larsoner

thanks for the quick feedback! We will still need some time to work on this, too many parallel things going on.

Okay! FWIW I don't think it would be too much work for me to push a commit or two to address my comments if it would help. Happy to do it if it makes your life easier! But also happy to wait if you're motivated to do the work.

eyelink synchronisation: we understand it is a bit specific, but it is implemented in MNE, so we added a new step to support it (80% ready for PR)

Ooof this one is trickier. From working on a project recently where I had to do this for EEG and MEG data that were recorded simultaneously on two different systems... it wasn't trivial. Sometimes events were missing from one system at the beginning or end, other times for the other system, etc. And even if they aren't, you somehow have to specify which events are shared between the two systems, via events or annotations, and which to keep or discard, etc. In the end for that dataset I synchronized manually and concatenated the instances before BIDSifying them. So I think this deserves an issue to discuss first at least...

zapline: this will be added to the filter thing as a quasi-replacement for 50/60Hz notch filtering, based on meegkit (only just started with the planning of that one).

Sure, I think this has been brought up in MNE-Python before, okay to add meegkit stuff here where it makes sense. I guess it would be inserted after the frequency filtering step but before regression, like between _04_frequency_filter.py and _05_regress_artifact.py?

mne_bids_pipeline/_config.py

pyproject.toml

behinger · 2025-02-12T20:33:50Z

ok, I tried to find time in the last month's and it didn't happen. I'd be happy if you can take a look and fix up some things if they are easy. I have some people in my team who could also take a look but o.c. aren't super familiar with the codebase

larsoner · 2025-02-13T20:10:08Z

Okay I'll try to look next week @behinger !

mpcoll · 2025-03-21T12:37:41Z

Thanks for all your work on this! I'm looking forward to seeing this functionality integrated into the pipeline.

While you're working on this, I wanted to suggest a potential enhancement: allowing users to only remove ICA components that were classified with sufficient confidence (using the "predict_proba" output from mne_icalabel). In my experience, removing everything that isn't classified as "brain" or "other" can sometimes be too conservative and may remove components unnecessarily.

This could be implemented by simply adding a probability threshold parameter in the configuration, where potentially artifactual components would only be removed if their classification confidence exceeds this threshold. This would give users more control over the artifact rejection process and potentially preserve more of the signal in ambiguous cases.

Thanks!

* upstream/main: (55 commits) [pre-commit.ci] pre-commit autoupdate (mne-tools#1099) FIX: sanitize tags (mne-tools#1097) [pre-commit.ci] pre-commit autoupdate (mne-tools#1098) [pre-commit.ci] pre-commit autoupdate (mne-tools#1096) [pre-commit.ci] pre-commit autoupdate (mne-tools#1095) Custom metadata (mne-tools#1088) fix mf_int_order; add mf_ext_order (mne-tools#1092) [pre-commit.ci] pre-commit autoupdate (mne-tools#1093) [pre-commit.ci] pre-commit autoupdate (mne-tools#1091) [pre-commit.ci] pre-commit autoupdate (mne-tools#1090) [pre-commit.ci] pre-commit autoupdate (mne-tools#1089) [pre-commit.ci] pre-commit autoupdate (mne-tools#1087) [pre-commit.ci] pre-commit autoupdate (mne-tools#1085) [pre-commit.ci] pre-commit autoupdate (mne-tools#1084) [pre-commit.ci] pre-commit autoupdate (mne-tools#1082) Properly compute rank (mne-tools#1069) [pre-commit.ci] pre-commit autoupdate (mne-tools#1080) avoid backslash in fstring and frontload fsaverage download (mne-tools#1078) fixup generation of template config files (mne-tools#1074) [pre-commit.ci] pre-commit autoupdate (mne-tools#1075) ...

for more information, see https://pre-commit.ci

…_label * jschepers/merge_ic_label: [pre-commit.ci] auto fixes from pre-commit.com hooks

larsoner · 2025-07-23T18:42:54Z

.circleci/config.yml

          name: Install dependencies
          command: |
-            pip install -ve .[docs]
+            pip install -ve .[docs] onnxruntime


I accidentally forgot this in one run and got a reasonable message:

FAILED mne_bids_pipeline/tests/test_run.py::test_run[ERP_CORE_N170] - ImportError: Missing optional dependency. ICLabel requires either pytorch or onnxruntime. Use pip or conda to install one of them.

so I think the error message propagated to the end user is working!

behinger · 2025-07-23T19:45:06Z

Thanks! We have internally some improvements to the code as well, I let the research assistant know to pr/merge with your changes.

larsoner · 2025-07-23T19:46:31Z

Yes feel free to push! I'm going to restart CIs but I expect them to come back green

jschepers added 8 commits June 26, 2024 09:34

added iclabel code from PR, user input which IC labels to keep, user …

2922224

…input whether to do eog or ecg detection on ICs

use brain and other as default for icalabel_include

3338807

added rudimentary plot of excluded components + labels + probabilities

4bf3976

Merge remote-tracking branch 'mne-bids-pipeline/ic_label' into merge_…

8eaf750

…ic_label

added iclabel code from PR, user input which IC labels to keep, user …

8ca9132

…input whether to do eog or ecg detection on ICs

use brain and other as default for icalabel_include

5b4d14e

added rudimentary plot of excluded components + labels + probabilities

fd304e1

Trying to resolve merge conflicts

ede148b

[pre-commit.ci] auto fixes from pre-commit.com hooks

7d2a729

for more information, see https://pre-commit.ci

larsoner reviewed Nov 7, 2024

View reviewed changes

larsoner reviewed Nov 12, 2024

View reviewed changes

mne_bids_pipeline/_config.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

larsoner and others added 6 commits July 23, 2025 10:54

BUG: Fixes

4a9f10a

WIP: Fixes

53d743f

[pre-commit.ci] auto fixes from pre-commit.com hooks

0070306

for more information, see https://pre-commit.ci

FIX: Comps

3e55605

Merge remote-tracking branch 'jschepers/merge_ic_label' into merge_ic…

c6bc2a3

…_label * jschepers/merge_ic_label: [pre-commit.ci] auto fixes from pre-commit.com hooks

larsoner marked this pull request as ready for review July 23, 2025 18:21

FIX: Need onnx

c2a4b6c

larsoner reviewed Jul 23, 2025

View reviewed changes

larsoner added 3 commits July 23, 2025 14:48

FIX: Clearer

ae5bb9d

FIX: Test

ea77558

FIX: Move

ec5dd1f

Merge branch 'main' into merge_ic_label

1d8e956

Add the code from PR "Add support for MNE-ICALabel #812" #1018

Are you sure you want to change the base?

Add the code from PR "Add support for MNE-ICALabel #812" #1018

Uh oh!

Conversation

jschepers commented Nov 7, 2024 • edited by larsoner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

welcome bot commented Nov 7, 2024

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsoner Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

behinger Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

larsoner Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

larsoner Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

behinger commented Nov 12, 2024

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

behinger commented Feb 12, 2025

Uh oh!

larsoner commented Feb 13, 2025

Uh oh!

mpcoll commented Mar 21, 2025

Uh oh!

larsoner Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

behinger commented Jul 23, 2025

Uh oh!

larsoner commented Jul 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jschepers commented Nov 7, 2024 •

edited by larsoner

Loading