BUG: Fix bug with BIDS split saving#12451
Merged
larsoner merged 11 commits intomne-tools:mainfrom Feb 21, 2024
Merged
Conversation
Member
|
+1 for proposal 4, raise an exception |
Member
|
+1 for iv as well, with raising a clearly written exception |
* upstream/main: [pre-commit.ci] pre-commit autoupdate (mne-tools#12453) MAINT: Fix CIs for PyQt6 (mne-tools#12452)
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! |
larsoner
commented
Feb 20, 2024
| @@ -0,0 +1 @@ | |||
| from .base import fetch_infant_template | |||
Member
Author
There was a problem hiding this comment.
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>
hoechenberger
approved these changes
Feb 21, 2024
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In mne-tools/mne-bids-pipeline#852 there is a bug where BIDSPath-based files were being written as:
This is because a mne-bids-pipeline naively did something like:
but it needed to have
split=Nonein theupdatecall to avoid the double_split-01_split-01.This is because currently MNE-Python takes
fname, converts it to apathlib.Path(automatically via dunderBIDSPath.__fspath__, which is an alias forstr(BIDSPath.fpath)), then checks to see if splits need to be done, and if so appends_split-01while 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:
Try to detect
mne_bids.BIDSPathinstances before converting to a path.If
bids_path.split is not None, do one of:bids_path.split, thereby still writing_split-01_split-01_epo.fif(backward compatible change; filename is invalid BIDS I think)bids_path.split = None, thereby writing_split-01_epo.fifor_epo.fifinstead depending on the data size (backward incompatible change)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)@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.fifor a_split-01_epo.fifcould be written depending on the file size).