Refactor test_epochs.py::test_split_saving (1 out of 2) by dmalt · Pull Request #11880 · mne-tools/mne-python

dmalt · 2023-08-14T16:51:42Z

Second attempt on PR #11876.

Start with refactoring tests related to saving epochs splits, while keeping the library code intact.

larsoner · 2023-08-14T17:04:57Z

ignore pip-pre for now, will be fixed by #11878

dmalt · 2023-08-14T17:23:57Z

mne/tests/test_epochs.py

+                }
+            )
+            epochs.metadata = metadata
        epochs.drop_bad()


drop_bad() is needed so assert len(epochs) == n_epochs doesn't break. before epochs.get_data() was calling drop_bads() behind the scenes.

larsoner · 2023-08-14T17:25:45Z

You can ignore CircleCI for now. To get it to stop being red feel free to add [circle skip] to your commits (but I'm happy enough to ignore-by-eye as well)

mne/tests/test_epochs.py

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

[skip ci]

dmalt · 2023-08-14T18:50:21Z

mne/tests/test_epochs.py

+@pytest.fixture(
+    params=[
+        ("1.5MB", 9, True, True, 6),
+        ("1.5MB", 9, True, False, 6),
+        ("1.5MB", 9, False, True, 6),
+        ("1.5MB", 9, False, False, 6),
+        ("3MB", 18, True, True, 3),
+        ("3MB", 18, True, False, 3),
+        ("3MB", 18, False, True, 3),
+        ("3MB", 18, False, False, 3),
+    ]
+)


Parametrisation now includes metadata and concat flags. I do it like this because n_files can potentially depend on these flags and I want to add corner case when these flags increase the number of splits later.

dmalt · 2023-08-14T19:04:55Z

mne/tests/test_epochs.py

+    epochs.save(
+        split_fpath, split_size="1.4MB", split_naming=split_naming, verbose=True
+    )
+
+    # check that the filenames match the intended pattern
+    assert split_fpath.is_file()
+    assert (tmp_path / split_fname_part1(n_files)).is_file()


number of splits with split_size="1.4MB" is completely unrelated to n_files, so checks in this test will break if data generation ever changes.
Also ...part1 in the variable name is misleading and n_files + 1 is wrong. I think it's supposed to check the last split, but the last split is n_files - 1.

dmalt · 2023-08-14T19:06:26Z

mne/tests/test_epochs.py

+    epochs, _, n_files = epochs_to_split
    split_fname = tmp_path / "test_epo.fif"
-    split_fname_neuromag_part1 = tmp_path / f"test_epo-{n_files + 1}.fif"
    split_fname_bids_part1 = tmp_path / f"test_split-{n_files + 1:02d}_epo.fif"


Same here. Part1 is really part_n and the indexing is wrong.

dmalt · 2023-08-14T19:13:06Z

So far the code should semantically be the same. Next I want to add some new stuff to the tests. @larsoner could you check up till now?

[skip ci]

larsoner · 2023-08-14T20:57:59Z

So far the code should semantically be the same. Next I want to add some new stuff to the tests. @larsoner could you check up till now?

Looks good so far. But please do wait for CIs to come back green before you add anything. Or I could mark for merge-when-green and you could continue in a follow-up PR (which would be a little easier review-wise since the main diff page can be used rather than having to look at single commits)

dmalt · 2023-08-14T21:42:42Z

Ok, let's merge when green then and I'll continue tomorrow in a follow-up PR.

larsoner · 2023-08-14T23:04:55Z

Done @dmalt , thanks for taking the time to make things easier to digest !

dmalt · 2023-08-15T09:12:00Z

No worries! I'm learning a lot. Besides, I like how it's coming out.

* upstream/main: 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)

* 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: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

dmalt marked this pull request as draft August 14, 2023 17:01

dmalt commented Aug 14, 2023

View reviewed changes

dmalt force-pushed the refactor-epochs-splits-naming-test branch 3 times, most recently from 07e02c5 to 2b0df72 Compare August 14, 2023 17:45

dmalt closed this Aug 14, 2023

dmalt reopened this Aug 14, 2023

dmalt force-pushed the refactor-epochs-splits-naming-test branch from 3bc5804 to da265fb Compare August 14, 2023 18:31

dmalt commented Aug 14, 2023

View reviewed changes

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

dmalt and others added 13 commits August 14, 2023 20:48

remove obsolete code

3f22229

remove size from parametrization

b396bf9

factor out epochs creation

543dbbe

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

0ad5f05

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

factor out split naming

d3625fe

parametrize over preload

e734acd

add docstring; enforce arrange-act-assert

becd7a8

ignore style error

b0afa5d

factor out overwrite test

b3647a0

factor out fname check when no splitting occurs

8eb2366

rewrite test_split_naming with parametrization

ab899cd

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

108f87e

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

remove os import

3aadc09

[skip ci]

dmalt force-pushed the refactor-epochs-splits-naming-test branch from 921ac90 to 3aadc09 Compare August 14, 2023 18:48

dmalt commented Aug 14, 2023

View reviewed changes

dmalt force-pushed the refactor-epochs-splits-naming-test branch from 655c2f8 to ef716f2 Compare August 14, 2023 20:07

dmalt and others added 2 commits August 14, 2023 22:08

Merge branch 'main' into refactor-epochs-splits-naming-test

ef716f2

[skip ci]

Merge branch 'main' into refactor-epochs-splits-naming-test

030f5f0

dmalt marked this pull request as ready for review August 14, 2023 21:42

dmalt changed the title ~~Refactor test_epochs.py::test_split_saving~~ Refactor test_epochs.py::test_split_saving (1 out of 2) Aug 14, 2023

larsoner merged commit 039122a into mne-tools:main Aug 14, 2023

dmalt mentioned this pull request Aug 15, 2023

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

Merged

dmalt deleted the refactor-epochs-splits-naming-test branch August 16, 2023 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Refactor test_epochs.py::test_split_saving (1 out of 2)#11880
larsoner merged 15 commits intomne-tools:mainfrom
dmalt:refactor-epochs-splits-naming-test

dmalt commented Aug 14, 2023 •

edited

Loading

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt Aug 14, 2023 •

edited

Loading

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

Uh oh!

dmalt Aug 14, 2023 •

edited

Loading

Uh oh!

dmalt Aug 14, 2023

Uh oh!

dmalt Aug 14, 2023

Uh oh!

dmalt commented Aug 14, 2023 •

edited

Loading

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt commented Aug 14, 2023

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt Aug 14, 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!

larsoner commented Aug 14, 2023

Uh oh!

Uh oh!

dmalt Aug 14, 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!

dmalt Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

dmalt Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt commented Aug 14, 2023

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

dmalt commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmalt commented Aug 14, 2023 •

edited

Loading

dmalt Aug 14, 2023 •

edited

Loading

dmalt Aug 14, 2023 •

edited

Loading

dmalt commented Aug 14, 2023 •

edited

Loading