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

ENH: Add eSSS #762

Merged
merged 13 commits into from
Jul 18, 2023
Merged

ENH: Add eSSS #762

merged 13 commits into from
Jul 18, 2023

Conversation

larsoner
Copy link
Member

Before merging …

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

Might not be 100% working yet but I think it's close

@larsoner
Copy link
Member Author

@drammock could be a good one for you to review and merge since it's pretty straightforward in terms of how to add something!

Comment on lines -146 to -150
# Now take everything from the bids_path_in and overwrite the parameters
subject = bids_path_in.subject # noqa: F841
session = bids_path_in.session # noqa: F841
run = bids_path_in.run

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a logging bug where it used to look like this:

[20:05:28] │ ⏳️ preprocessing/_03_maxfilter sub-01 Saving report: /home/larsoner/mne_data/derivatives/mne-bids-pipeline/ds003392/sub-01/meg/sub-01_task-localizer_report.html
[20:05:28] │ ⏳️ preprocessing/_03_maxfilter sub-emptyroom ses-19111211 run-noise Loading reference run: None.

instead of this:

[20:12:13] │ ⏳️ preprocessing/_03_maxfilter sub-01 Saving report: /home/larsoner/mne_data/derivatives/mne-bids-pipeline/ds003392/sub-01/meg/sub-01_task-localizer_report.html
[20:12:13] │ ⏳️ preprocessing/_03_maxfilter sub-01 run-noise Loading reference run: None.

The logging of which file/step we're on makes more sense now I think, and is also consistent with how _01_data_quality and _04_frequency_filter were already logging:

[20:11:40] │ ⏳️ preprocessing/_01_data_quality sub-01 Saving report: /home/larsoner/mne_data/derivatives/mne-bids-pipeline/ds003392/sub-01/meg/sub-01_task-localizer_report.html
[20:11:40] │ ⏳️ preprocessing/_01_data_quality sub-01 run-noise Finding flat channels and noisy channels using Maxwell filtering.
...
[20:12:37] │ ⏳️ preprocessing/_04_frequency_filter sub-01 Saving report: /home/larsoner/mne_data/derivatives/mne-bids-pipeline/ds003392/sub-01/meg/sub-01_task-localizer_report.html
[20:12:38] │ ⏳️ preprocessing/_04_frequency_filter sub-01 run-noise Reading empty-room recording: sub-01_task-noise_proc-sss_raw.fif

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

looks reasonable! I'm sure you know I'm not yet familiar enough with the pipeline internals to catch everything, but at least nothing is glaringly wrong, the report looks reasonable and the tests are passing, so +1 for merge.

import gc
from typing import Optional
from types import SimpleNamespace

import numpy as np
import mne
from mne.utils import _pl
Copy link
Member

Choose a reason for hiding this comment

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

of all the cases of using a private function outside its package, this is quite possibly the most innocuous. But still I feel it's my duty to say that if we're going to use _pl outside of MNE-Python then we should make it public.

Copy link
Member Author

Choose a reason for hiding this comment

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

And this too:

mne_bids_pipeline/_config_import.py:from mne.utils import _check_option, _validate_type

but I can at least replicate the _pl here to avoid that one. Maybe check_option and validate_type should be public at some point... but I've always been annoyed their signatures don't have the name/val in the same order, so making them public feels dissatisfying :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay got rid of _pl and made #766 to mention _check_option and _validate_type

run, task = None, "noise"
in_key = f"raw_task-{task}_run-{run}"
bids_path_in = in_files.pop(in_key)
bids_path_bads_in = in_files.pop(f"{in_key}-bads", None) # noqa
Copy link
Member

Choose a reason for hiding this comment

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

this is F841 (assigned but never used): why not assign to underscore? I see the commented-out TODO below but don't really understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because eventually we should use this (with the given variable filename) but we have an unaddressed bug. So to me it helps to have the variable name we should be using here, and it commented out (with that same name) below to at least establish the link (which you did implicitly already, even if you didn't understand the "why aren't we actually using it" part!)

mne_bids_pipeline/steps/preprocessing/_03_maxfilter.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger enabled auto-merge (squash) July 18, 2023 16:28
@hoechenberger hoechenberger merged commit 62dde41 into mne-tools:main Jul 18, 2023
7 checks passed
@larsoner larsoner deleted the esss branch July 18, 2023 17:31
larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Jul 18, 2023
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.

4 participants