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.

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

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

Merged
merged 30 commits into from
Aug 16, 2023

Conversation

dmalt
Copy link
Contributor

@dmalt dmalt commented Aug 15, 2023

Continuation of #11880

Leg work to fix #11870.

  • Make adding new test cases more straightforward via parametrisation
  • Fix problems with existing tests
  • Add test cases to stress problems with existing implementation

@dmalt dmalt marked this pull request as draft August 15, 2023 09:27
@dmalt dmalt changed the title Refactor test_epochs.py::test_split_saving (1 out of 2) Refactor test_epochs.py::test_split_saving (2 out of 2) Aug 15, 2023
@dmalt dmalt force-pushed the refactor-epochs-splits-naming-test-2 branch from f325dd5 to 4764a17 Compare August 15, 2023 11:08
@dmalt dmalt force-pushed the refactor-epochs-splits-naming-test-2 branch from 38da5c5 to 2559126 Compare August 15, 2023 11:18
pre-commit-ci bot and others added 3 commits August 15, 2023 13:20
Recently added check for the number of saved files in test_split_naming,
fails on Windows: the amount of splits on Windows differs from expected.
Reduce the number of epochs to make this test pass.

[skip ci]
Instead of saving epochs to create existing file, use `touch()`;
this gives finer control over corner cases. Use parametrization to cover
these cases. N.B.: apparently there's a bug.

[ci skip]
@dmalt dmalt force-pushed the refactor-epochs-splits-naming-test-2 branch from 180d91e to cde329b Compare August 15, 2023 18:13
@dmalt dmalt marked this pull request as ready for review August 15, 2023 18:57
@dmalt dmalt marked this pull request as draft August 15, 2023 18:59
@dmalt dmalt marked this pull request as ready for review August 15, 2023 19:13
@dmalt
Copy link
Contributor Author

dmalt commented Aug 15, 2023

Ready for review as soon as the tests pass. test_split_naming still might fail for some parametrisation cases on some pipelines. If that happens, I'll adjust the data. This happens because I added a case, where size is close to a splitting threshold and file sizes differ a bit in different environments, so there can be a mismatch between expected and real number of splits .

@dmalt dmalt force-pushed the refactor-epochs-splits-naming-test-2 branch from 314c742 to d0bc779 Compare August 16, 2023 12:07
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Just one minor gripe which I'll commit, which will also get all CIs to run.

FYI for CircleCI can you try going to the site and logging in with GitHub? It should make it happy to build the docs for you

mne/tests/test_epochs.py Outdated Show resolved Hide resolved
@larsoner larsoner enabled auto-merge (squash) August 16, 2023 16:24
auto-merge was automatically disabled August 16, 2023 16:46

Head branch was pushed to by a user without write access

@dmalt
Copy link
Contributor Author

dmalt commented Aug 16, 2023

Just one minor gripe which I'll commit, which will also get all CIs to run.

Great, thanks!

FYI for CircleCI can you try going to the site and logging in with GitHub? It should make it happy to build the docs for you

Looks like it worked.

@larsoner larsoner enabled auto-merge (squash) August 16, 2023 16:51
auto-merge was automatically disabled August 16, 2023 17:56

Head branch was pushed to by a user without write access

@dmalt
Copy link
Contributor Author

dmalt commented Aug 16, 2023

@larsoner looks like ci/circleci: pytest-macos-arm64 is stuck. Can you merge it before main diverges, so I can start working on the fix?

@larsoner larsoner merged commit 75c83c3 into mne-tools:main Aug 16, 2023
24 checks passed
@larsoner
Copy link
Member

Thanks @dmalt !

larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 16, 2023
* upstream/main:
  Refactor test_epochs.py::test_split_saving (2 out of 2) (mne-tools#11884)
  Cross-figure event passing system (mne-tools#11685)
larsoner added a commit to drammock/mne-python that referenced this pull request Aug 22, 2023
* 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)
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
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.

Inconsistent splits naming between Raw and Epochs when using BIDS schema
2 participants