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

BUG: Fix several bugs #839

Merged
merged 10 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/source/v1.6.md.inc
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion mne_bids_pipeline/_config_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
29 changes: 21 additions & 8 deletions mne_bids_pipeline/_config_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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]]:
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -573,7 +586,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(
Expand Down
1 change: 0 additions & 1 deletion mne_bids_pipeline/_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
Expand Down
10 changes: 5 additions & 5 deletions mne_bids_pipeline/_reject.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
14 changes: 12 additions & 2 deletions mne_bids_pipeline/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,10 +379,20 @@ def _prep_out_files(
exec_params: SimpleNamespace,
out_files: dict[str, BIDSPath],
):
# Don't look at just deriv_root as this ends with <path>/mne-bids-pipeline and
# sometimes we write stuff to <path>/freesurfer
rel_root = exec_params.deriv_root.parent
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(rel_root):
raise RuntimeError(
f"Output BIDSPath not relative to deriv_root parent {rel_root}:"
f"\n{fname}"
)
out_files[key] = _path_to_str_hash(
key,
pathlib.Path(fname),
fname,
method=exec_params.memory_file_method,
kind="out",
)
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
Expand Down
19 changes: 14 additions & 5 deletions mne_bids_pipeline/steps/preprocessing/_09_ptp_reject.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,10 @@ 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"
kwargs = dict(section=title, tags=tags, replace=True)
with _open_report(
cfg=cfg, exec_params=exec_params, subject=subject, session=session
) as report:
Expand All @@ -201,19 +205,24 @@ 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"),
replace=True,
**kwargs,
)
del caption
else:
report.add_html(
html=f"<code>{reject}</code>",
title=f"{kind} thresholds",
**kwargs,
)

report.add_epochs(
epochs=epochs,
title="Epochs: after cleaning",
title=title,
psd=psd,
drop_log_ignore=(),
replace=True,
**kwargs,
)
return _prep_out_files(exec_params=exec_params, out_files=out_files)

Expand Down
Loading