BUG: Fix epoch splits naming by dmalt · Pull Request #11876 · mne-tools/mne-python

dmalt · 2023-08-12T18:50:09Z

Fixes overwriting bug, when epoch.save('test-epo.fif', overwrite=False, split_naming="neuromag") would overwrite
"test-epo-1.fif".
Disallows filenames like "test-epo.fif" with split_naming="bids" to avoid surprises with split names. File names like
"a_b-epo.fif", i.e. having several bids clauses divided by "_" are still allowed. Also we don't do this check if no splits will be created.

agramfort

let me know when it's good to go from your end. Don't forget to update latest.inc file in the doc. thx

mne/tests/test_epochs.py

dmalt · 2023-08-13T08:39:51Z

While working on the PR, found another problem: if the ending is not BIDS, i.e. -epo.fif and split_naming="bids", epochs.save() produces files like _split-01_test-epo.fif without complains. Should I prohibit -epo.fif with split_naming="bids"? I think raising ValueError in this case would be reasonable.

Also, should epochs.save("test-epo.fif", split_naming="bids") fail if no splits will be produced?

dmalt · 2023-08-13T09:19:23Z

mne/tests/test_epochs.py

-        with pytest.raises(FileExistsError, match="Destination file"):
-            epochs.save(split_fname, split_naming=split_naming, verbose=True)
-    os.remove(split_fname)
-    # we don't test for reserved files as it's not implemented here


I don't know what reserved files means here. I noticed, Raw.save() mentions something reserving the main file when producing splits. Is it something important I should be aware of? For now I just removed this comment.

dmalt · 2023-08-13T09:21:51Z

mne/tests/test_epochs.py

    events = mne.make_fixed_length_events(raw, 1)
    epochs = mne.Epochs(raw, events)
-    if split_size == "2MB" and (metadata or concat):
-        n_files += 1


Not sure what this was doing. Removed it.

This was a special case inside the test. Basically it was necessary because in the case where your split_size is small and you have metadata (or concat, for whatever reason), you will end up with another file. It doesn't seem like your PR should change the file sizes produced but rather just the filenames, so it seems like a bug (either on this PR or in main currently) that you were able to remove this and have things still pass.

... looks like it is a bug on main because we never use split_size="2MB", so removing this seems okay

dmalt · 2023-08-13T09:26:26Z

mne/tests/test_epochs.py

@@ -1511,50 +1511,102 @@ def test_split_saving(tmp_path, split_size, n_epochs, n_files, size, metadata, c
            }
        )
        epochs.metadata = metadata


I don't see, how the metadata are used in the tests. Do metadata affect epochs.drop_bad()? I don't think they do. Same thing with concat. Maybe they should be removed.

They were probably added to make sure that we actually split the metadata properly (or copy it? not sure which we do currently) when splitting the epoch data across multiple files

I've looked into commit history and it seems they are added to fix #7897. My current understanding is that there was a bug with saving splits when the size just marginally exceeded the splitting threshold, so metadata and concat manipulations help catch this corner case. Does this make sense?

Also there's #5102 related to problem with events when loading back the splits.

there was a bug with saving splits when the size just marginally exceeded the splitting threshold

ah yes I remember that one. Nice git archeology!

larsoner · 2023-08-14T14:59:11Z

if the ending is not BIDS, i.e. -epo.fif and split_naming="bids", epochs.save() produces files like _split-01_test-epo.fif without complains. Should I prohibit -epo.fif with split_naming="bids"? I think raising ValueError in this case would be reasonable.

@sappelhoff WDYT the BIDS-sensible thing is to do in this case?

larsoner

@dmalt thanks for working on this!

As it stands, this PR is a little bit tough to review because you're mixing adding functionality with refactoring non-trivial tests that cover weird corner cases. It is hard to see what has been changed to make tests cleaner/better while keeping all existing functionalityl/checks (this is difficult to see) vs what was changed to accommodate the new naming scheme. It makes me worry that we might silently have break/omit some test that we put in place previously to cover some corner case. So even though the tests look cleaner, in some sense they are a bit less safe now.

I would suggest reverting the test changes, then start by adding just a new parametrization like:

@pytest.mark.parametrize("split_naming", ("neuromag", "bids"))

on a test or a few tests. Then you'll need to make a few small naming updates in the tests. This would be digestible from a review standpoint.

Alternatively you could open a fresh PR that only does the test refactoring without adding anything new. This could include stuff like the removal of the if split_size == "2MB" that probably doesn't need to be there. Then we get that green and merged, then rebase this PR on that... and then this PR once again hopefully just adds a parametrize + naming updates as above.

Does that make sense?

dmalt · 2023-08-14T16:38:26Z

Yes, you're right, the PR spiralled out of control real quick:) Let me try from scratch. I feel like refactoring tests first would be a better option. The library code changes are easy but testing them without bloating an existing test even more -- not as much.

sappelhoff · 2023-08-14T20:23:13Z

@sappelhoff WDYT the BIDS-sensible thing is to do in this case?

IMHO mne-bids is the BIDS-aware software and the feature that is provided via MNE-Python here is more a convenience for mne-bids. So I don't have a strong opinion as we correctly use this in mne-bids.

I wouldn't get into too much of a logic branching here and simply assume that users will either:

need to know what they are doing or
use mne-bids

dmalt · 2023-08-17T08:18:55Z

@sappelhoff WDYT the BIDS-sensible thing is to do in this case?

IMHO mne-bids is the BIDS-aware software and the feature that is provided via MNE-Python here is more a convenience for mne-bids. So I don't have a strong opinion as we correctly use this in mne-bids.

I wouldn't get into too much of a logic branching here and simply assume that users will either:

need to know what they are doing or

use mne-bids

I was thinking adding a simple check fname.endswith("_epo.fif") when using bids just to avoid accidents. Does that sound reasonable?

dmalt · 2023-08-17T15:51:09Z

I was thinking adding a simple check fname.endswith("_epo.fif") when using bids just to avoid accidents. Does that sound reasonable?

As a matter of fact, this check is already implemented. It was just turned off. The filenames for splits are constructed via mne.io.utils._construct_bids_filename called validate=False. validate=True would do what we need here (almost).

dmalt · 2023-08-17T15:55:08Z

mne/epochs.py

+        fname = f"{base}-{part_idx:d}{ext}"
+    elif split_naming == "bids" and n_parts > 1:
+        fname = _construct_bids_filename(base, ext, part_idx + 1)
+    _check_fname(fname, overwrite=overwrite)


call _check_fname() for both 'neuromag' and 'bids' to fix overwriting bug when
epochs.save('test-epo.fif', overwrite=False) would overwrite existing 'test-epo-1.fif'

dmalt · 2023-08-17T15:56:13Z

mne/epochs.py

+    if part_idx > 0 and split_naming == "neuromag":
+        fname = f"{base}-{part_idx:d}{ext}"
+    elif split_naming == "bids" and n_parts > 1:
+        fname = _construct_bids_filename(base, ext, part_idx + 1)


remove validate=False to prohibit using '-epo.fif' with split_naming='bids'

dmalt · 2023-08-17T16:04:23Z

This is ready from my side.
I'm not sure if my last commit belongs here, or if it should go to a separate PR. I felt like it would be easier to convince yourself that the implementation is correct on the refactored version. If you disagree, feel free to revert the last commit or ask me and I'll do it.

Use already present validation mechanism. For now validation only works when splits will be created, i.e. test-epo.fif with split_naming="bids" is still allowed. Record this behaviour in tests.

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

larsoner

Much easier

mne/io/utils.py

mne/tests/test_epochs.py

- add xfail reasons - add match regex to pytest.raises - remove elif, add _check_option

for more information, see https://pre-commit.ci

mne/tests/test_epochs.py

doc/changes/devel.rst

drammock · 2023-08-17T19:44:41Z

@larsoner our "check rendered docs here" CI isn't working? takes me to the same link as the CircleCI Details link, and there is no artifact to view

larsoner · 2023-08-17T19:58:21Z

It also doesn't run pytest-macos-arm64 -- @dmalt when I asked to sign up for CircleCI did you do something like enable it for your repo? It should be using MNE-Python's runs for CircleCI but it's using your user's. It's messing up the linking and preventing pytest-macos-arm64 from running, neither of which are total blockers but would be good to know how this happened...

larsoner · 2023-08-17T19:58:56Z

(By "sign up" I just meant to "log in with GitHub" on the UI when you click "Details", so if you did more than that it would be good to know!)

Co-authored-by: Daniel McCloy <dan@mccloy.info>

drammock · 2023-08-17T20:03:47Z

OK well I guess no harm in re-running them with the changelog commit then. I'll commit that and then mark for merge when green

drammock · 2023-08-17T20:04:12Z

ha, @larsoner beats me to it again :)

* upstream/main: BUG: Fix epoch splits naming (mne-tools#11876)

dmalt · 2023-08-17T20:18:08Z

It also doesn't run pytest-macos-arm64 -- @dmalt when I asked to sign up for CircleCI did you do something like enable it for your repo? It should be using MNE-Python's runs for CircleCI but it's using your user's. It's messing up the linking and preventing pytest-macos-arm64 from running, neither of which are total blockers but would be good to know how this happened...

Honestly, no idea. Sorry :( All I did is logging in with GitHub. It's my first experience with Circle CI, so I might have clicked something by accident.

larsoner · 2023-08-17T20:20:47Z

Honestly, no idea. Sorry :( All I did is logging in with GitHub.

Sounds like what you did should have worked, weird!

dmalt · 2023-08-17T20:28:29Z

Honestly, no idea. Sorry :( All I did is logging in with GitHub.

Sounds like what you did should have worked, weird!

And I still have this problem, right? For my next PR I mean.

larsoner · 2023-08-17T20:33:44Z

Yes probably... maybe there is some way you can disable CircleCI building on your fork, you can try messing with settings in https://app.circleci.com/pipelines/github/dmalt/mne-python

dmalt · 2023-08-17T20:43:20Z

Yes probably... maybe there is some way you can disable CircleCI building on your fork, you can try messing with settings in https://app.circleci.com/pipelines/github/dmalt/mne-python

Ok, I think the problem was that I clicked "Set Up Project" next to MNE-Python right after logging in. To be fair, the button was right in the middle of a screen, so it was calling for action:) I deleted the project and created a test PR #11897 and now it seems to be working fine.

* upstream/main: [pre-commit.ci] pre-commit autoupdate (mne-tools#11911) [BUG, MRG] Remove check on `mne.viz.Brain.add_volume_labels` (mne-tools#11889) Small splits fix (mne-tools#11905) adds niseq package to "Related software" (mne-tools#11909) Minor fixes for ERDS maps example (mne-tools#11904) FIX: Fix pyvista rendering (mne-tools#11896) BUG: Fix epoch splits naming (mne-tools#11876) ENH: Use section-title for HTML anchors in Report (mne-tools#11890) CI: Deploy [circle deploy] MAINT: Clean up whats_new and doc versions (mne-tools#11888) Refactor test_epochs.py::test_split_saving (2 out of 2) (mne-tools#11884) Cross-figure event passing system (mne-tools#11685) MAINT: Post-release deprecations, updates [circle deploy] (mne-tools#11887) MAINT: Release 1.5.0 (mne-tools#11886) [pre-commit.ci] pre-commit autoupdate (mne-tools#11883) Refactor test_epochs.py::test_split_saving (1 out of 2) (mne-tools#11880) FIX: Missing Saccade information in Eyelink File (mne-tools#11877) Improve drawing of annotations with matplotlib (mne-tools#11855) MAINT: Work around NumPy deprecation (mne-tools#11878)

Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>

agramfort reviewed Aug 13, 2023

View reviewed changes

mne/tests/test_epochs.py Outdated Show resolved Hide resolved

mne/tests/test_epochs.py Outdated Show resolved Hide resolved

dmalt marked this pull request as draft August 13, 2023 08:46

dmalt commented Aug 13, 2023

View reviewed changes

larsoner reviewed Aug 14, 2023

View reviewed changes

dmalt mentioned this pull request Aug 14, 2023

Refactor test_epochs.py::test_split_saving (1 out of 2) #11880

Merged

dmalt force-pushed the fix-epoch-splits-naming branch from 997074b to 651517c Compare August 16, 2023 22:54

dmalt commented Aug 17, 2023

View reviewed changes

dmalt marked this pull request as ready for review August 17, 2023 15:56

dmalt changed the title ~~WIP: Fix epoch splits naming~~ BUG: Fix epoch splits naming Aug 17, 2023

dmalt force-pushed the fix-epoch-splits-naming branch 2 times, most recently from 0097cfb to 91b4a04 Compare August 17, 2023 16:42

dmalt and others added 6 commits August 17, 2023 18:44

fix bids splits naming for epochs

3632249

ENH: Use section-title for HTML anchors in Report (mne-tools#11890)

6aca98e

disallow -epo.fif with bids split_naming; test it

8c1fc59

Use already present validation mechanism. For now validation only works when splits will be created, i.e. test-epo.fif with split_naming="bids" is still allowed. Record this behaviour in tests.

fix style

a9a12e8

mark test as xfailing

ed5d136

refactor _save_split()

91b4a04

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

larsoner reviewed Aug 17, 2023

View reviewed changes

mne/io/utils.py Outdated Show resolved Hide resolved

mne/tests/test_epochs.py Outdated Show resolved Hide resolved

mne/tests/test_epochs.py Outdated Show resolved Hide resolved

dmalt force-pushed the fix-epoch-splits-naming branch from a5fb3e2 to 5a9ddeb Compare August 17, 2023 17:56

dmalt and others added 2 commits August 17, 2023 19:58

address review comments

5a9ddeb

- add xfail reasons - add match regex to pytest.raises - remove elif, add _check_option

[pre-commit.ci] auto fixes from pre-commit.com hooks

37a83d8

for more information, see https://pre-commit.ci

larsoner reviewed Aug 17, 2023

View reviewed changes

mne/tests/test_epochs.py Outdated Show resolved Hide resolved

Update mne/tests/test_epochs.py

90a0eb5

Merge branch 'main' into fix-epoch-splits-naming

abe4edb

larsoner approved these changes Aug 17, 2023

View reviewed changes

add to changelog

f4c059d

drammock approved these changes Aug 17, 2023

View reviewed changes

drammock enabled auto-merge (squash) August 17, 2023 19:40

drammock reviewed Aug 17, 2023

View reviewed changes

doc/changes/devel.rst Outdated Show resolved Hide resolved

drammock disabled auto-merge August 17, 2023 19:42

Update doc/changes/devel.rst [ci skip]

9b73e12

Co-authored-by: Daniel McCloy <dan@mccloy.info>

larsoner merged commit 9e85b2e into mne-tools:main Aug 17, 2023

larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 17, 2023

Merge remote-tracking branch 'upstream/main' into fxaa

6b38f62

* upstream/main: BUG: Fix epoch splits naming (mne-tools#11876)

dmalt deleted the fix-epoch-splits-naming branch August 17, 2023 20:47

Uh oh!

Conversation

dmalt commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmalt commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

dmalt commented Aug 14, 2023

Uh oh!

sappelhoff commented Aug 14, 2023

Uh oh!

dmalt commented Aug 17, 2023

Uh oh!

dmalt commented Aug 17, 2023

Uh oh!

dmalt Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmalt commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drammock commented Aug 17, 2023

Uh oh!

larsoner commented Aug 17, 2023

Uh oh!

larsoner commented Aug 17, 2023

Uh oh!

drammock commented Aug 17, 2023

Uh oh!

drammock commented Aug 17, 2023

Uh oh!

dmalt commented Aug 17, 2023

Uh oh!

larsoner commented Aug 17, 2023

Uh oh!

dmalt commented Aug 17, 2023

Uh oh!

larsoner commented Aug 17, 2023

Uh oh!

dmalt commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

dmalt commented Aug 12, 2023 •

edited

Loading

dmalt commented Aug 13, 2023 •

edited

Loading

dmalt Aug 17, 2023 •

edited

Loading

dmalt commented Aug 17, 2023 •

edited

Loading

dmalt commented Aug 17, 2023 •

edited

Loading