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.

Sign up for GitHub

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

Jump to bottom

[BUG, MRG] Remove check on mne.viz.Brain.add_volume_labels #11889

Merged
merged 3 commits into from
Aug 22, 2023
Merged

[BUG, MRG] Remove check on mne.viz.Brain.add_volume_labels #11889

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
536b400
remove check
alexrockhill Aug 15, 2023
0fe0ee1
more permissive, still error
alexrockhill Aug 21, 2023
cd778a8
Merge branch 'main' into no_error
alexrockhill Aug 21, 2023
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
2 changes: 0 additions & 2 deletions mne/viz/_brain/_brain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2628,8 +2628,6 @@ def add_volume_labels(
import nibabel as nib

# load anatomical segmentation image
if not aseg.endswith("aseg"):
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a safer approach would be:

if not aseg.endswith(("aseg", "wmparc")):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't check this kind of stuff elsewhere i.e. if you try and read a trans file, it doesn't check that it ends with trans.fif so maybe better just to let any readable file

Copy link
Member

Choose a reason for hiding this comment

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

We don't check this kind of stuff elsewhere

Actually we do -- we have a filename checking function to do this sort of thing for raw/epochs/evoked. Given our tight coupling with FreeSurfer and how infrequently they add new stuff, I don't mind having to change the check every few years to accommodate a new name. In the meantime if we get rid of the check we risk users accidentally passing files that don't work and getting some cryptic error later. There's no telling how many times the current check has saved a user some pain but I bet it's a number greater than zero somewhere...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, okay to change to a warning though?

Copy link
Member

@larsoner larsoner Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Lets default to error and allow overriding with _on_missing style handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used endswith parc or aseg to be slightly more permissive without adding an extra argument which I'm not sure is worth the added complexity

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a safer approach would be:

if not aseg.endswith(("aseg", "wmparc")):

raise RuntimeError(f'`aseg` file path must end with "aseg", got {aseg}')
aseg = str(
_check_fname(
op.join(
Expand Down
Loading