-
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
BUG: Fix bug with CSP computation and Maxwell filter #890
Conversation
@SophieHerbst feel free to try it, computation appears to be working locally |
@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): |
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.
This was not working properly and is now
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") |
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.
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)
plt.close(fig) | ||
del fig, title |
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.
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: |
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 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.
if not len(time_bins): | ||
fname_csp_cluster_results = None | ||
else: | ||
time_bins = pd.DataFrame(time_bins, columns=["t_min", "t_max"]) |
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.
@hoechenberger I suggest reviewing this PR with "hide whitespace changes" because the following lines are all just indented
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Thanks @larsoner! |
It works now, thank you @larsoner and @hoechenberger! |
@SophieHerbst @larsoner I just cut a new release (1.8.0) containing this fix |
Before merging …
docs/source/changes.md
)Closes #882
Closes #876
Also updated the doc of
decoding_csp_times = None
as this is not a no-op.