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

don't fail if emptyroom is missing cHPI channels #977

Merged
merged 8 commits into from
Jul 31, 2024

Conversation

drammock
Copy link
Member

@drammock drammock commented Jul 30, 2024

closes #974

Before merging …

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

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looking at:

$ git grep mf_filter
...
mne_bids_pipeline/tests/configs/config_ds004229.py:mf_filter_chpi = False  # for speed, not needed as we low-pass anyway

maybe having this True would have caught the error on main. Do you want to check? If not I can try quickly.

Either way might be good to set to True assuming in only adds a reasonable amount of time on CircleCI (e.g., < 1 minute more is reasonable, > 3 minutes probably isn't).

docs/source/v1.10.md.inc Outdated Show resolved Hide resolved
@larsoner
Copy link
Member

maybe having this True would have caught the error on main. Do you want to check? If not I can try quickly.

Locally

rm -Rf ~/mne_data/derivatives/mne-bids-pipeline/ds004229/ && time pytest mne_bids_pipeline/ -k 4229

took 5m50.472s. Setting mf_filter_chpi = True on main did emit an error!

I also realized that we should filter cHPI before performing movement compensation, so I changed that order and added it in the changelog as a bugfix.

Marking for merge-when-green, thanks in advance @drammock !

@drammock
Copy link
Member Author

took 5m50.472s. Setting mf_filter_chpi = True on main did emit an error!

heh, you beat me to it, was just about to check for that failure on main

@larsoner
Copy link
Member

Nice 🙂 And just for the sake of completeness, I'll add that changing it to True had a negligible effect on testing time (took 5m49.272s which is a tiny bit shorter than before, so definitely in the test-retest speed standard deviation somewhere).

@larsoner larsoner enabled auto-merge (squash) July 31, 2024 16:15
@larsoner larsoner merged commit ea5545d into mne-tools:main Jul 31, 2024
49 of 50 checks passed
@drammock drammock deleted the allow-line-only branch July 31, 2024 17:38
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.

pipeline fails if emptyroom data is missing cHPI
2 participants