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 bug with CSP computation and Maxwell filter #890

Merged
merged 11 commits into from
Mar 15, 2024

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Mar 14, 2024

Before merging …

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

Closes #882
Closes #876

Also updated the doc of decoding_csp_times = None as this is not a no-op.

@larsoner
Copy link
Member Author

@SophieHerbst feel free to try it, computation appears to be working locally

@larsoner
Copy link
Member Author

@SophieHerbst this should also now fix #876

@@ -67,13 +67,13 @@ def __mne_bids_pipeline_failsafe_wrapper__(*args, **kwargs):
# Find the limit / step where the error occurred
step_dir = pathlib.Path(__file__).parent / "steps"
tb = traceback.extract_tb(e.__traceback__)
for fi, frame in enumerate(inspect.stack()):
for fi, frame in enumerate(tb):
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 was not working properly and is now

Comment on lines -82 to -87
epochs_filt = epochs.copy().pick(["meg", "eeg"])

# We only take mag to speed up computation
# because the information is redundant between grad and mag
if cfg.datatype == "meg" and cfg.use_maxwell_filter:
epochs_filt.pick("mag")
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to pick at all here because it's done elsewhere, and we use PCA to reduce dimensionality (so the redundancy between mag/grad doesn't matter anyway)

Comment on lines +494 to +495
plt.close(fig)
del fig, title
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid warning about unclosed figures

@@ -16,9 +13,3 @@ def _write_json(fname: PathLike, data: dict) -> None:
def _read_json(fname: PathLike) -> dict:
with open(fname, encoding="utf-8") as f:
return json_tricks.load(f)


def _empty_room_match_path(run_path: BIDSPath, cfg: SimpleNamespace) -> BIDSPath:
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this to _import_data.py which is a better place for it. _io.py now just has to do with generic file I/O (currently just json) rather than our chose (BIDS)Path structures.

mne_bids_pipeline/_config.py Outdated Show resolved Hide resolved
if not len(time_bins):
fname_csp_cluster_results = None
else:
time_bins = pd.DataFrame(time_bins, columns=["t_min", "t_max"])
Copy link
Member Author

Choose a reason for hiding this comment

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

@hoechenberger I suggest reviewing this PR with "hide whitespace changes" because the following lines are all just indented

docs/source/v1.8.md.inc Outdated Show resolved Hide resolved
mne_bids_pipeline/_config.py Show resolved Hide resolved
mne_bids_pipeline/_decoding.py Outdated Show resolved Hide resolved
mne_bids_pipeline/_run.py Outdated Show resolved Hide resolved
mne_bids_pipeline/_run.py Outdated Show resolved Hide resolved
mne_bids_pipeline/steps/sensor/_99_group_average.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger merged commit 45e4c13 into mne-tools:main Mar 15, 2024
52 checks passed
@hoechenberger
Copy link
Member

Thanks @larsoner!

@SophieHerbst
Copy link
Collaborator

It works now, thank you @larsoner and @hoechenberger!

@hoechenberger
Copy link
Member

@SophieHerbst @larsoner I just cut a new release (1.8.0) containing this fix

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.

CSP decoding error: all fits failed run-noise Output file hash mismatch for _task-noise_scores.json
3 participants