-
Notifications
You must be signed in to change notification settings - Fork 67
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
ENH: Add artifact regression #837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some inline comments to facilitate eventual review
@@ -449,7 +449,6 @@ def import_er_data( | |||
cfg=cfg, | |||
bids_path_bads=bids_path_er_bads_in, | |||
) | |||
raw_er.pick("meg", exclude=[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason we were only picking MEG channels in empty room data. To me this step seems unnecessary, and also breaks artifact regression, so I think we should drop it.
msg = "Adding config and sys info to report" | ||
logger.info(**gen_log_kwargs(message=msg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this later in the file where we can make sure we emit it exactly once per mne_bids_pipeline
call (other calls should be fast and it's not helpful to have dozens of these messages in the logs)
subject=subject, # save under subject's directory so all files are there | ||
session=session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the filt
step for SSS processing would output to sub-01/*
type paths, whereas if SSS was off it would output to sub-emptyroom/*
type paths. This unifies it so everything lands in subject-specific paths. This means that after the filt
step (e.g., during regression, SSP, and ICA) we can assume all files for each subject come from that subject's path rather than potentially sub-emptyroom
. This may mean a few redundant files but simplifies logic and should help with #831.
@@ -199,6 +204,18 @@ def filter_data( | |||
check=False, | |||
) | |||
|
|||
if cfg.regress_artifact is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to filter the data and regression channels the same way
|
||
_STEPS = ( | ||
_01_data_quality, | ||
_02_head_pos, | ||
_03_maxfilter, | ||
_04_frequency_filter, | ||
_05_make_epochs, | ||
_05_regress_artifact, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the order was SSS, filt, epoch, SSP/ICA, clean epochs. Now the order is SSS, filt, SSP/ICA, epoch, clean epochs. This makes it clearer that the epochs are not used for SSP and ICA, which is helpful in terms of understanding the flow in that we basically stay working with raw data ountil step _07 now.
@@ -202,7 +213,7 @@ | |||
covariance (via `noise_cov='rest'`). | |||
""" | |||
|
|||
ch_types: Iterable[Literal["meg", "mag", "grad", "eeg"]] = [] | |||
ch_types: Annotated[Sequence[Literal["meg", "mag", "grad", "eeg"]], Len(1, 4)] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I errantly had ch_types = "meg"
. By changing this from Iterable
to Sequence
, I got a nice error message saying it couldn't just be a string. Then we can use pydantic
to further validate the length such as non-emptiness using Len
from annotated_types
, which is required by pydantic
. So the deletions in _config_import.py
below are now handled by pydantic for us!
.circleci/config.yml
Outdated
@@ -48,7 +48,7 @@ jobs: | |||
name: Get Python running | |||
command: | | |||
pip install --upgrade --progress-bar off pip | |||
pip install --upgrade --progress-bar off "autoreject @ https://api.github.com/repos/autoreject/autoreject/zipball/master" "mne[hdf5] @ git+https://github.com/mne-tools/mne-python@main" "mne-bids[full] @ https://api.github.com/repos/mne-tools/mne-bids/zipball/main" numba | |||
pip install --upgrade --progress-bar off "autoreject @ https://api.github.com/repos/autoreject/autoreject/zipball/master" "mne[hdf5] @ git+https://github.com/larsoner/mne-python@regress" "mne-bids[full] @ https://api.github.com/repos/mne-tools/mne-bids/zipball/main" numba |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily use my branch until mne-tools/mne-python#12389 lands
... @hoechenberger I think this is ready for review. If you're happy I'll wait until mne-tools/mne-python#12389 lands, revert the change to use my branch, then merge |
Seems to work, nice low-freq noise suppressed by regressing the ref sensors! I'll work on fixing the conflicts then should be good to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @larsoner! |
Before merging …
docs/source/changes.md
)Adds
config.regress_artifact
, used in my pipeline to regress reference sensor signals:Draft because I need to find a good KIT dataset to process to show this stuff, any ideas @agramfort ? I saw MASC-MEG but it's continuous speech (which we probably don't handle well) and huge.
I might try converting https://mne.tools/dev/auto_tutorials/inverse/95_phantom_KIT.html to BIDS since it works to demonstrate the processing.
Closes #832