Skip to content

Conversation

@larsoner
Copy link
Member

In mne-tools/mne-bids-pipeline#852 there is a bug where BIDSPath-based files were being written as:

sub-155_task-tiwm_proc-ica_split-01_split-01_epo.fif

This is because a mne-bids-pipeline naively did something like:

fname = BIDSPath(..., split="01")
epochs = read_epochs(fname)
...
epochs_out.save(fname.copy().update(proc="something))

but it needed to have split=None in the update call to avoid the double _split-01_split-01.

This is because currently MNE-Python takes fname, converts it to a pathlib.Path (automatically via dunder BIDSPath.__fspath__, which is an alias for str(BIDSPath.fpath)), then checks to see if splits need to be done, and if so appends _split-01 while writing. So you end up with a filename ..._split-01_split-01_epo.fif.

So far this PR just exposes the issue (true TDD!). My proposed solution is to:

  1. Try to detect mne_bids.BIDSPath instances before converting to a path.

  2. If bids_path.split is not None, do one of:

    1. Emit a warning, but keep existing bids_path.split, thereby still writing _split-01_split-01_epo.fif (backward compatible change; filename is invalid BIDS I think)
    2. Emit a warning, but replace bids_path.split = None, thereby writing _split-01_epo.fif or _epo.fif instead depending on the data size (backward incompatible change)
    3. Emit a warning only if a split file will be written and bids_path.split not in (None, "01"), and replace bids_path.split = None` (backward incompatible change; seems fragile because the warning behavior depends on stuff like the size of the data being saved to disk)
    4. Raise an error, thereby writing nothing (backward incompatible change; most explicit)

@sappelhoff @hoechenberger any opinion on which is best here? I'm inclined toward option (iv) / raising an error because it forces the user in their code to do a .update(split=None) when writing a file, which makes things more explicit for them (i.e., makes it clear a _epo.fif or a _split-01_epo.fif could be written depending on the file size).

@hoechenberger
Copy link
Member

+1 for proposal 4, raise an exception

@sappelhoff
Copy link
Member

+1 for iv as well, with raising a clearly written exception

@larsoner larsoner marked this pull request as ready for review February 20, 2024 20:13
@larsoner
Copy link
Member Author

Okay pushed a test fix. Also removed our dependency on pytest-harvest since what we use it for is very basic (hardly added any new code for us to avoid using it) and it's not fully pytest 8.0.0 + python12 compatible anyway. @sappelhoff feel free to review when you have a chance!

@@ -0,0 +1 @@
from .base import fetch_infant_template
Copy link
Member Author

Choose a reason for hiding this comment

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

Snuck this in there because with latest pytest dev I was getting some weird error about submodule nesting... this makes _infant somehow appear as a more proper module so that it doesn't complain.

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@larsoner larsoner enabled auto-merge (squash) February 21, 2024 13:57
@larsoner larsoner merged commit a867e5c into mne-tools:main Feb 21, 2024
@larsoner larsoner deleted the split branch February 21, 2024 14:32
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
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.

3 participants