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
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

Before merging …

Changelog has been updated (docs/source/dev.md.inc)

…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

larsoner · 2024-11-07T17:48:41Z

mne_bids_pipeline/_config.py

+    ```python
+    eeg_reference = "average"
+    ica_l_freq = 1
+    h_freq = 100


We might want to add a ica_h_freq param

that would be a breaking change then?

Nah doesn't have to be -- we can use ice_h_freq=None as the default (as None is what we use now), and people wanting to use icalabel would need to set it to 100.

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!

larsoner · 2024-11-07T17:51:47Z

mne_bids_pipeline/steps/preprocessing/_06a1_fit_ica.py

+    if cfg.ica_use_icalabel:
+        # The ICALabel network was trained on extended-Infomax ICA decompositions fit
+        # on data flltered between 1 and 100 Hz.
+        assert cfg.ica_algorithm in ["picard-extended_infomax", "extended_infomax"]


An equivalent check should probably live up in _config_import.py

larsoner · 2024-11-07T17:53:14Z

mne_bids_pipeline/_config.py

+    ```
+"""
+
+icalabel_include: Annotated[


I think to be consistent all ICA-related stuff should be prefixed by ica_ so this should be

Suggested change

icalabel_include: Annotated[

ica_icalabel_include: Annotated[

larsoner · 2024-11-07T19:20:31Z

pyproject.toml

@@ -48,6 +48,8 @@ dependencies = [
  "autoreject",
  "mne[hdf5] >=1.7",
  "mne-bids[full]",
+  "mne-icalabel",
+  "onnxruntime",  # for mne-icalabel


... shouldn't this be in the deps of mne-icalabel? At least maybe in an optional form like mne-icalabel[something]?

I'm a bit confused on this one, it is an explicit requirement for mne-icalabel:
https://mne.tools/mne-icalabel/stable/install.html#dependencies (pypi-tab)
but one could choose pytorch as well. My gutfeeling was that pytorch is the larger one of these two

Ohhh this:

The ICLabel model requires either pytorch or Microsoft onnxruntime.

I don't think we should make this choice for people in mne-bids-pipeline requirements (i.e., shouldn't do it in our pyproject.toml), and instead tell them to install one or the other, just like mne-icalabel does. And in our CIs we can do that (either PyTorch or onnxruntime)

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?

larsoner · 2024-11-12T17:04:48Z

mne_bids_pipeline/_config.py

+    ```python
+    eeg_reference = "average"
+    ica_l_freq = 1
+    h_freq = 100


Nah doesn't have to be -- we can use ice_h_freq=None as the default (as None is what we use now), and people wanting to use icalabel would need to set it to 100.

larsoner · 2024-11-12T17:07:02Z

pyproject.toml

@@ -48,6 +48,8 @@ dependencies = [
  "autoreject",
  "mne[hdf5] >=1.7",
  "mne-bids[full]",
+  "mne-icalabel",
+  "onnxruntime",  # for mne-icalabel


Ohhh this:

The ICLabel model requires either pytorch or Microsoft onnxruntime.

I don't think we should make this choice for people in mne-bids-pipeline requirements (i.e., shouldn't do it in our pyproject.toml), and instead tell them to install one or the other, just like mne-icalabel does. And in our CIs we can do that (either PyTorch or onnxruntime)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jschepers commented Nov 7, 2024 •

edited

Loading

welcome bot commented Nov 7, 2024

larsoner left a comment

larsoner Nov 7, 2024

behinger Nov 12, 2024

larsoner Nov 12, 2024

larsoner Nov 7, 2024

behinger Nov 12, 2024

larsoner Nov 12, 2024

larsoner Nov 7, 2024

larsoner Nov 7, 2024

larsoner Nov 7, 2024

behinger Nov 12, 2024

larsoner Nov 12, 2024

behinger commented Nov 12, 2024

larsoner left a comment

larsoner Nov 12, 2024

larsoner Nov 12, 2024

	icalabel_include: Annotated[
	ica_icalabel_include: Annotated[

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

Conversation

jschepers commented Nov 7, 2024 • edited Loading

Changes

Before merging …

welcome bot commented Nov 7, 2024

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behinger commented Nov 12, 2024

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschepers commented Nov 7, 2024 •

edited

Loading