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
133 stars 65 forks source link

Fix handling of `analyze_channels` for EEG data, where some steps (like ERP calculation) would previously fail #883

Closed hoechenberger closed 4 months ago

hoechenberger commented 4 months ago

This was reported at https://mne.discourse.group/t/error-when-setting-analyze-channels-using-mne-bids-pipeline

Before merging …

larsoner commented 4 months ago

For stuff like this as much as possible I still try to follow TDD and modify an example/test config to show the problem. Maybe one of the ERP_CORE datasets:

$ git grep "eeg_reference = "
...
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = "average"
mne_bids_pipeline/tests/configs/config_ERP_CORE.py:    eeg_reference = ["P9", "P10"]
...

And it looks like analyze_channels is completely untested!

$ git grep "analyze_channels = "
mne_bids_pipeline/_config.py:    analyze_channels = ['Pz']
mne_bids_pipeline/_config_utils.py:        analyze_channels = cfg.analyze_channels
mne_bids_pipeline/_config_utils.py:            analyze_channels = cfg.ch_types

So we should probably add it here. @hoechenberger feel free to try it on main to ensure it fails then add to this PR, or I can do it.

hoechenberger commented 4 months ago

@larsoner Yes absolutely, I forgot to mention that I was actually planning to add a test case

I will do that when I have time; changing to a draft PR now!

hoechenberger commented 4 months ago

@larsoner Both, N170 and N2pc, fail to run on main with the configuration settings used here (reproduces the bug locally for me); everything is green on this PR branch. This is ready for review & merge

larsoner commented 4 months ago

Marking for merge-when-green, thanks @hoechenberger !

hoechenberger commented 4 months ago

@larsoner Think we can cut a 1.7 release with this?

larsoner commented 4 months ago

Sure or a 1.6.1

hoechenberger commented 4 months ago

@larsoner I tagged a new release but the doc deployment is failing :( https://app.circleci.com/pipelines/github/mne-tools/mne-bids-pipeline/4328/workflows/045c8d95-d657-4020-9ecf-71c00f53e2d9/jobs/60266/parallel-runs/0/steps/0-109

larsoner commented 4 months ago

Looks like:

remote: fatal: pack exceeds maximum allowed size (2.00 GiB)        

one solution is to archive or just remove some old doc versions like 1.0, 1.1, etc.

larsoner commented 4 months ago

@hoechenberger WDYT about deleting 1.0 and 1.1?

hoechenberger commented 4 months ago

I'm also fine with keeping only the last 2 or 3 releases + dev :)

larsoner commented 4 months ago

FYI to fix things I re-ran the failed step with SSH (though I guess I could have done it locally), then SSH'ed in and did:

$ source $BASH_ENV
$ cd project
$ git checkout main  # was on gh-pages
$ mike list --config-file docs/mkdocs.yml
Generating option->step mapping: 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████| 56/56 [00:00<00:00, 228.61it/s]
warning: gh-pages is unrelated to origin/gh-pages
dev
1.7 [stable]
1.6
1.5
1.4
1.3
1.2
1.1
1.0
$ mike delete 1.0 --config-file docs/mkdocs.yml --ignore-remote-status
... # same for 1.1, 1.2, 1.3, and 1.4
$ git checkout gh-pages
$ git reset $(git commit-tree HEAD^{tree} -m "Deploy and squash docs [ci skip]")
$ git push origin --force gh-pages
...
Writing objects: 100% (1041/1041), 1.88 GiB | 70.96 MiB/s, done.
...
 + 9988ef4...ad8b0d3 gh-pages -> gh-pages (forced update)

and now things look okay:

https://mne.tools/mne-bids-pipeline/stable/

dev might have gotten reset but that should be fixed the next time we merge, which will hopefully be soon enough.

Note that we do get this warning in deployment:

Warning: Uploaded artifact size of 2875160017 bytes exceeds the allowed size of 1 GB. Deployment might fail.

So we might have more compression / size reduction to figure out at some point