From a65f278d92eb7d36914d3c630209dce12a4c6a8a Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 30 Jan 2024 14:37:03 -0500 Subject: [PATCH] BUG: Fix several bugs (#839) --- docs/source/v1.6.md.inc | 4 ++ mne_bids_pipeline/_config_import.py | 2 +- mne_bids_pipeline/_config_utils.py | 40 ++++++++++--------- mne_bids_pipeline/_main.py | 1 - mne_bids_pipeline/_reject.py | 10 ++--- mne_bids_pipeline/_run.py | 14 ++++++- .../steps/freesurfer/_02_coreg_surfaces.py | 22 +++++++--- .../preprocessing/_05_regress_artifact.py | 1 - .../steps/preprocessing/_09_ptp_reject.py | 19 +++++++-- .../steps/source/_01_make_bem_surfaces.py | 4 +- .../steps/source/_02_make_bem_solution.py | 4 +- .../steps/source/_03_setup_source_space.py | 4 +- 12 files changed, 85 insertions(+), 40 deletions(-) diff --git a/docs/source/v1.6.md.inc b/docs/source/v1.6.md.inc index afb7835c3..01bfd87e4 100644 --- a/docs/source/v1.6.md.inc +++ b/docs/source/v1.6.md.inc @@ -5,6 +5,7 @@ :new: New features & enhancements - Added [`regress_artifact`][mne_bids_pipeline._config.regress_artifact] to allow artifact regression (e.g., of MEG reference sensors in KIT systems) (#837 by @larsoner) +- Chosen `reject` parameters are now saved in the generated HTML reports (#839 by @larsoner) [//]: # (### :warning: Behavior changes) @@ -17,6 +18,9 @@ ### :bug: Bug fixes - Fix minor issues with path handling for cross-talk and calibration files (#834 by @larsoner) +- Fix bug where EEG `reject` params were not used for `ch_types = ["meg", "eeg"]` (#839 by @larsoner) +- Fix bug where implicit `mf_reference_run` could change across invocations of `mne_bids_pipeline`, breaking caching (#839 by @larsoner) +- Fix bug where `--no-cache` had no effect (#839 by @larsoner) ### :medical_symbol: Code health diff --git a/mne_bids_pipeline/_config_import.py b/mne_bids_pipeline/_config_import.py index db5487cb7..fa8fb6772 100644 --- a/mne_bids_pipeline/_config_import.py +++ b/mne_bids_pipeline/_config_import.py @@ -369,7 +369,7 @@ def _pydantic_validate( if config_path is not None: name += f" from {config_path}" model_config = ConfigDict( - arbitrary_types_allowed=False, + arbitrary_types_allowed=True, # needed in 2.6.0 to allow DigMontage for example validate_assignment=True, strict=True, # do not allow float for int for example extra="forbid", diff --git a/mne_bids_pipeline/_config_utils.py b/mne_bids_pipeline/_config_utils.py index 321ccf0f0..7b555a2a4 100644 --- a/mne_bids_pipeline/_config_utils.py +++ b/mne_bids_pipeline/_config_utils.py @@ -16,9 +16,9 @@ from .typing import ArbitraryContrast try: - _keys_arbitrary_contrast = set(ArbitraryContrast.__required_keys__) + _set_keys_arbitrary_contrast = set(ArbitraryContrast.__required_keys__) except Exception: - _keys_arbitrary_contrast = set(ArbitraryContrast.__annotations__.keys()) + _set_keys_arbitrary_contrast = set(ArbitraryContrast.__annotations__.keys()) def get_fs_subjects_dir(config: SimpleNamespace) -> pathlib.Path: @@ -96,11 +96,14 @@ def get_subjects(config: SimpleNamespace) -> list[str]: else: s = config.subjects - subjects = set(s) - set(config.exclude_subjects) - # Drop empty-room subject. - subjects = subjects - set(["emptyroom"]) + # Preserve order and remove excluded subjects + subjects = [ + subject + for subject in s + if subject not in config.exclude_subjects and subject != "emptyroom" + ] - return sorted(subjects) + return subjects def get_sessions(config: SimpleNamespace) -> Union[list[None], list[str]]: @@ -176,7 +179,17 @@ def _get_runs_all_subjects_cached( def get_intersect_run(config: SimpleNamespace) -> list[str]: """Return the intersection of all the runs of all subjects.""" subj_runs = get_runs_all_subjects(config) - return list(set.intersection(*map(set, subj_runs.values()))) + # Do not use something like: + # list(set.intersection(*map(set, subj_runs.values()))) + # as it will not preserve order. Instead just be explicit and preserve order. + # We could use "sorted", but it's probably better to use the order provided by + # the user (if they want to put `runs=["02", "01"]` etc. it's better to use "02") + all_runs = list() + for runs in subj_runs.values(): + for run in runs: + if run not in all_runs: + all_runs.append(run) + return all_runs def get_runs( @@ -429,17 +442,6 @@ def _restrict_analyze_channels( return inst -def _get_scalp_in_files(cfg: SimpleNamespace) -> dict[str, pathlib.Path]: - subject_path = pathlib.Path(cfg.subjects_dir) / cfg.fs_subject - seghead = subject_path / "surf" / "lh.seghead" - in_files = dict() - if seghead.is_file(): - in_files["seghead"] = seghead - else: - in_files["t1"] = subject_path / "mri" / "T1.mgz" - return in_files - - def _get_bem_conductivity(cfg: SimpleNamespace) -> tuple[tuple[float], str]: if cfg.fs_subject in ("fsaverage", cfg.use_template_mri): conductivity = None # should never be used @@ -573,7 +575,7 @@ def _validate_contrasts(contrasts: SimpleNamespace) -> None: if len(contrast) != 2: raise ValueError("Contrasts' tuples MUST be two conditions") elif isinstance(contrast, dict): - if not _keys_arbitrary_contrast.issubset(set(contrast.keys())): + if not _set_keys_arbitrary_contrast.issubset(set(contrast.keys())): raise ValueError(f"Missing key(s) in contrast {contrast}") if len(contrast["conditions"]) != len(contrast["weights"]): raise ValueError( diff --git a/mne_bids_pipeline/_main.py b/mne_bids_pipeline/_main.py index 04ddabe1e..56d14a010 100755 --- a/mne_bids_pipeline/_main.py +++ b/mne_bids_pipeline/_main.py @@ -141,7 +141,6 @@ def main(): steps = (steps,) on_error = "debug" if debug else None - cache = "1" if cache else "0" processing_stages = [] processing_steps = [] diff --git a/mne_bids_pipeline/_reject.py b/mne_bids_pipeline/_reject.py index ca506239d..707984732 100644 --- a/mne_bids_pipeline/_reject.py +++ b/mne_bids_pipeline/_reject.py @@ -45,11 +45,11 @@ def _get_reject( # Only keep thresholds for channel types of interest reject = reject.copy() - if ch_types == ["eeg"]: - ch_types_to_remove = ("mag", "grad") - else: - ch_types_to_remove = ("eeg",) - + ch_types_to_remove = list() + if "meg" not in ch_types: + ch_types_to_remove.extend(("mag", "grad")) + if "eeg" not in ch_types: + ch_types_to_remove.append("eeg") for ch_type in ch_types_to_remove: try: del reject[ch_type] diff --git a/mne_bids_pipeline/_run.py b/mne_bids_pipeline/_run.py index c7e46267b..04deef839 100644 --- a/mne_bids_pipeline/_run.py +++ b/mne_bids_pipeline/_run.py @@ -378,11 +378,21 @@ def _prep_out_files( *, exec_params: SimpleNamespace, out_files: dict[str, BIDSPath], + check_relative: Optional[pathlib.Path] = None, ): + if check_relative is None: + check_relative = exec_params.deriv_root for key, fname in out_files.items(): + # Sanity check that we only ever write to the derivatives directory + fname = pathlib.Path(fname) + if not fname.is_relative_to(check_relative): + raise RuntimeError( + f"Output BIDSPath not relative to expected root {check_relative}:" + f"\n{fname}" + ) out_files[key] = _path_to_str_hash( key, - pathlib.Path(fname), + fname, method=exec_params.memory_file_method, kind="out", ) @@ -401,7 +411,7 @@ def _path_to_str_hash( assert isinstance(v, pathlib.Path), f'Bad type {type(v)}: {kind}_files["{k}"] = {v}' assert v.exists(), f'missing {kind}_files["{k}"] = {v}' if method == "mtime": - this_hash = v.lstat().st_mtime + this_hash = v.stat().st_mtime else: assert method == "hash" # guaranteed this_hash = hash_file_path(v) diff --git a/mne_bids_pipeline/steps/freesurfer/_02_coreg_surfaces.py b/mne_bids_pipeline/steps/freesurfer/_02_coreg_surfaces.py index eb5f86151..b2e2f8090 100644 --- a/mne_bids_pipeline/steps/freesurfer/_02_coreg_surfaces.py +++ b/mne_bids_pipeline/steps/freesurfer/_02_coreg_surfaces.py @@ -10,7 +10,6 @@ import mne.bem from ..._config_utils import ( - _get_scalp_in_files, get_fs_subject, get_fs_subjects_dir, get_subjects, @@ -22,6 +21,17 @@ fs_bids_app = Path(__file__).parent / "contrib" / "run.py" +def _get_scalp_in_files(cfg: SimpleNamespace) -> dict[str, Path]: + subject_path = Path(cfg.fs_subjects_dir) / cfg.fs_subject + seghead = subject_path / "surf" / "lh.seghead" + in_files = dict() + if seghead.is_file(): + in_files["seghead"] = seghead + else: + in_files["t1"] = subject_path / "mri" / "T1.mgz" + return in_files + + def get_input_fnames_coreg_surfaces( *, cfg: SimpleNamespace, @@ -32,7 +42,7 @@ def get_input_fnames_coreg_surfaces( def get_output_fnames_coreg_surfaces(*, cfg: SimpleNamespace, subject: str) -> dict: out_files = dict() - subject_path = Path(cfg.subjects_dir) / cfg.fs_subject + subject_path = Path(cfg.fs_subjects_dir) / cfg.fs_subject out_files["seghead"] = subject_path / "surf" / "lh.seghead" for key in ("dense", "medium", "sparse"): out_files[f"head-{key}"] = ( @@ -57,19 +67,21 @@ def make_coreg_surfaces( in_files.pop("t1" if "t1" in in_files else "seghead") mne.bem.make_scalp_surfaces( subject=cfg.fs_subject, - subjects_dir=cfg.subjects_dir, + subjects_dir=cfg.fs_subjects_dir, force=True, overwrite=True, ) out_files = get_output_fnames_coreg_surfaces(cfg=cfg, subject=subject) - return _prep_out_files(exec_params=exec_params, out_files=out_files) + return _prep_out_files( + exec_params=exec_params, out_files=out_files, check_relative=cfg.fs_subjects_dir + ) def get_config(*, config, subject) -> SimpleNamespace: cfg = SimpleNamespace( subject=subject, fs_subject=get_fs_subject(config, subject), - subjects_dir=get_fs_subjects_dir(config), + fs_subjects_dir=get_fs_subjects_dir(config), ) return cfg diff --git a/mne_bids_pipeline/steps/preprocessing/_05_regress_artifact.py b/mne_bids_pipeline/steps/preprocessing/_05_regress_artifact.py index 8a2b2a0f6..5ab1119a6 100644 --- a/mne_bids_pipeline/steps/preprocessing/_05_regress_artifact.py +++ b/mne_bids_pipeline/steps/preprocessing/_05_regress_artifact.py @@ -76,7 +76,6 @@ def run_regress_artifact( out_files["regress"] = bids_path_in.copy().update( processing=None, split=None, - run=None, suffix="regress", extension=".h5", ) diff --git a/mne_bids_pipeline/steps/preprocessing/_09_ptp_reject.py b/mne_bids_pipeline/steps/preprocessing/_09_ptp_reject.py index 7f0bf0607..3584aa72f 100644 --- a/mne_bids_pipeline/steps/preprocessing/_09_ptp_reject.py +++ b/mne_bids_pipeline/steps/preprocessing/_09_ptp_reject.py @@ -187,6 +187,9 @@ def drop_ptp( psd = True else: psd = 30 + tags = ("epochs", "reject") + kind = cfg.reject if isinstance(cfg.reject, str) else "Rejection" + title = "Epochs: after cleaning" with _open_report( cfg=cfg, exec_params=exec_params, subject=subject, session=session ) as report: @@ -201,18 +204,28 @@ def drop_ptp( fig=reject_log.plot( orientation="horizontal", aspect="auto", show=False ), - title="Epochs: Autoreject cleaning", + title=f"{kind} cleaning", caption=caption, - tags=("epochs", "autoreject"), + section=title, + tags=tags, replace=True, ) del caption + else: + report.add_html( + html=f"{reject}", + title=f"{kind} thresholds", + section=title, + replace=True, + tags=tags, + ) report.add_epochs( epochs=epochs, - title="Epochs: after cleaning", + title=title, psd=psd, drop_log_ignore=(), + tags=tags, replace=True, ) return _prep_out_files(exec_params=exec_params, out_files=out_files) diff --git a/mne_bids_pipeline/steps/source/_01_make_bem_surfaces.py b/mne_bids_pipeline/steps/source/_01_make_bem_surfaces.py index da2b64890..f77593107 100644 --- a/mne_bids_pipeline/steps/source/_01_make_bem_surfaces.py +++ b/mne_bids_pipeline/steps/source/_01_make_bem_surfaces.py @@ -112,7 +112,9 @@ def make_bem_surfaces( subject=subject, session=session, ) - return _prep_out_files(exec_params=exec_params, out_files=out_files) + return _prep_out_files( + exec_params=exec_params, out_files=out_files, check_relative=cfg.fs_subjects_dir + ) def get_config( diff --git a/mne_bids_pipeline/steps/source/_02_make_bem_solution.py b/mne_bids_pipeline/steps/source/_02_make_bem_solution.py index a09d063e2..33f7b870c 100644 --- a/mne_bids_pipeline/steps/source/_02_make_bem_solution.py +++ b/mne_bids_pipeline/steps/source/_02_make_bem_solution.py @@ -69,7 +69,9 @@ def make_bem_solution( out_files = get_output_fnames_make_bem_solution(cfg=cfg, subject=subject) mne.write_bem_surfaces(out_files["model"], bem_model, overwrite=True) mne.write_bem_solution(out_files["sol"], bem_sol, overwrite=True) - return _prep_out_files(exec_params=exec_params, out_files=out_files) + return _prep_out_files( + exec_params=exec_params, out_files=out_files, check_relative=cfg.fs_subjects_dir + ) def get_config( diff --git a/mne_bids_pipeline/steps/source/_03_setup_source_space.py b/mne_bids_pipeline/steps/source/_03_setup_source_space.py index 64e7314ed..52c342dbf 100644 --- a/mne_bids_pipeline/steps/source/_03_setup_source_space.py +++ b/mne_bids_pipeline/steps/source/_03_setup_source_space.py @@ -55,7 +55,9 @@ def run_setup_source_space( in_files.clear() # all used by setup_source_space out_files = get_output_fnames_setup_source_space(cfg=cfg, subject=subject) mne.write_source_spaces(out_files["src"], src, overwrite=True) - return _prep_out_files(exec_params=exec_params, out_files=out_files) + return _prep_out_files( + exec_params=exec_params, out_files=out_files, check_relative=cfg.fs_subjects_dir + ) def get_config(