Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

hoechenberger
Copy link
Member

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

Before merging …

  • Changelog has been updated (docs/source/changes.md)

@hoechenberger hoechenberger changed the title Fix hadling of analyze_channels for EEG data, where some steps (like ERP calculation) would previously fail Fix handling of analyze_channels for EEG data, where some steps (like ERP calculation) would previously fail Mar 12, 2024
@larsoner
Copy link
Member

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
Copy link
Member Author

@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 hoechenberger marked this pull request as draft March 12, 2024 14:54
Comment on lines 261 to +264
eeg_reference = "average"
# Analyze all EEG channels -- we only specify the channels here for the purpose of
# demonstration
analyze_channels = [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test with average reference

Comment on lines 214 to +217
eeg_reference = ["P9", "P10"]
# Analyze all EEG channels -- we only specify the channels here for the purpose of
# demonstration
analyze_channels = [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test with custom reference

@hoechenberger hoechenberger marked this pull request as ready for review March 12, 2024 16:11
@hoechenberger
Copy link
Member Author

hoechenberger commented Mar 12, 2024

@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 larsoner enabled auto-merge (squash) March 12, 2024 17:05
@larsoner
Copy link
Member

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

@larsoner larsoner merged commit 60e6a54 into mne-tools:main Mar 12, 2024
49 of 50 checks passed
@hoechenberger
Copy link
Member Author

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

@larsoner
Copy link
Member

Sure or a 1.6.1

@hoechenberger
Copy link
Member Author

@larsoner
Copy link
Member

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.

@hoechenberger hoechenberger deleted the eeg-reference branch March 13, 2024 18:38
@larsoner
Copy link
Member

@hoechenberger WDYT about deleting 1.0 and 1.1?

@hoechenberger
Copy link
Member Author

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

@larsoner
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants