From c8320d118618259e5205ff865555c0101e80513d Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Mon, 19 Feb 2024 12:56:38 -0500 Subject: [PATCH 01/10] BUG: Fix bug with BIDS split saving --- mne/tests/test_epochs.py | 30 +++++++++++++++++++++++++----- pyproject.toml | 1 + 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/mne/tests/test_epochs.py b/mne/tests/test_epochs.py index 015974e89cc..266781b1491 100644 --- a/mne/tests/test_epochs.py +++ b/mne/tests/test_epochs.py @@ -1666,44 +1666,64 @@ def test_split_saving_and_loading_back(tmp_path, epochs_to_split, preload): @pytest.mark.parametrize( - "split_naming, dst_fname, split_fname_fn", + "split_naming, dst_fname, split_fname_fn, check_bids", [ ( "neuromag", "test_epo.fif", lambda i: f"test_epo-{i}.fif" if i else "test_epo.fif", + False, ), ( "bids", - "test_epo.fif", - lambda i: f"test_split-{i + 1:02d}_epo.fif", + Path("sub-01") / "meg" / "sub-01_epo.fif", + lambda i: Path("sub-01") / "meg" / f"sub-01_split-{i + 1:02d}_epo.fif", + True, ), ( "bids", "a_b-epo.fif", # Merely stating the fact: lambda i: f"a_split-{i + 1:02d}_b-epo.fif", + False, ), ], ids=["neuromag", "bids", "mix"], ) def test_split_naming( - tmp_path, epochs_to_split, split_naming, dst_fname, split_fname_fn + tmp_path, epochs_to_split, split_naming, dst_fname, split_fname_fn, check_bids ): """Test naming of the split files.""" epochs, split_size, n_files = epochs_to_split dst_fpath = tmp_path / dst_fname save_kwargs = {"split_size": split_size, "split_naming": split_naming} # we don't test for reserved files as it's not implemented here + if dst_fpath.parent != tmp_path: + dst_fpath.parent.mkdir(parents=True) epochs.save(dst_fpath, verbose=True, **save_kwargs) # check that the filenames match the intended pattern - assert len(list(tmp_path.iterdir())) == n_files + assert len(list(dst_fpath.parent.iterdir())) == n_files for i in range(n_files): assert (tmp_path / split_fname_fn(i)).is_file() assert not (tmp_path / split_fname_fn(n_files)).is_file() + if not check_bids: + return + # If we load sub-01_split-01_epo.fif we should then be able to write it + # without it being named sub-01_split-01_split-01_epo.fif + mne_bids = pytest.importorskip("mne_bids") + # Let's try to prevent people from making a mistake + bids_path = mne_bids.BIDSPath( + root=tmp_path, subject="01", split="01", suffix="epo", check=False + ) + assert bids_path.fpath.is_file(), bids_path.fpath + epochs.save(bids_path, verbose=True, overwrite=True, **save_kwargs) + bad_path = bids_path.fpath.parent / (bids_path.fpath.stem[:-3] + "split-01_epo.fif") + assert str(bad_path).count("_split-01") == 2 + assert not bad_path.is_file(), bad_path # TODO: fails! + @pytest.mark.parametrize( "dst_fname, split_naming, split_1_fname", diff --git a/pyproject.toml b/pyproject.toml index 998ceffc5e2..8b145929c86 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -138,6 +138,7 @@ test_extra = [ "imageio-ffmpeg>=0.4.1", "snirf", "neo", + "mne_bids", ] # Dependencies for building the docuemntation From 17638a1060afe34ae12a0de9e8a38b92e953ff98 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 11:44:15 -0500 Subject: [PATCH 02/10] FIX: Implement --- doc/changes/devel/12451.bugfix.rst | 1 + mne/epochs.py | 9 ++++++++- mne/io/base.py | 4 +++- mne/io/fiff/tests/test_raw_fiff.py | 26 ++++++++++++++++++++++++++ mne/tests/test_epochs.py | 30 +++++++++++++++++++++++------- mne/utils/check.py | 20 +++++++++++++++++++- 6 files changed, 80 insertions(+), 10 deletions(-) create mode 100644 doc/changes/devel/12451.bugfix.rst diff --git a/doc/changes/devel/12451.bugfix.rst b/doc/changes/devel/12451.bugfix.rst new file mode 100644 index 00000000000..11c5c6496af --- /dev/null +++ b/doc/changes/devel/12451.bugfix.rst @@ -0,0 +1 @@ +Fix ambiguous and likely errant redundant use of ``BIDSPath.split`` when writing split raw and epochs data, by `Eric Larson`_. diff --git a/mne/epochs.py b/mne/epochs.py index 7da05dbd045..7b87dee5179 100644 --- a/mne/epochs.py +++ b/mne/epochs.py @@ -2212,7 +2212,14 @@ def save( ) # check for file existence and expand `~` if present - fname = str(_check_fname(fname=fname, overwrite=overwrite)) + fname = str( + _check_fname( + fname=fname, + overwrite=overwrite, + check_bids_split=True, + name="fname", + ) + ) split_size_bytes = _get_split_size(split_size) diff --git a/mne/io/base.py b/mne/io/base.py index 4fe7975e1cd..46953f6809c 100644 --- a/mne/io/base.py +++ b/mne/io/base.py @@ -1694,7 +1694,9 @@ def save( endings_err = (".fif", ".fif.gz") # convert to str, check for overwrite a few lines later - fname = _check_fname(fname, overwrite=True, verbose="error") + fname = _check_fname( + fname, overwrite=True, verbose="error", check_bids_split=True + ) check_fname(fname, "raw", endings, endings_err=endings_err) split_size = _get_split_size(split_size) diff --git a/mne/io/fiff/tests/test_raw_fiff.py b/mne/io/fiff/tests/test_raw_fiff.py index cb1626a4ef7..200a7906b8b 100644 --- a/mne/io/fiff/tests/test_raw_fiff.py +++ b/mne/io/fiff/tests/test_raw_fiff.py @@ -675,6 +675,32 @@ def test_split_files(tmp_path, mod, monkeypatch): assert not fname_3.is_file() +def test_bids_split_files(tmp_path): + """Test that BIDS split files are written safely.""" + mne_bids = pytest.importorskip("mne_bids") + bids_path = mne_bids.BIDSPath( + root=tmp_path, + subject="01", + datatype="meg", + split="01", + suffix="raw", + extension=".fif", + check=False, + ) + (tmp_path / "sub-01" / "meg").mkdir(parents=True) + raw = read_raw_fif(test_fif_fname) + save_kwargs = dict( + buffer_size_sec=1.0, split_size="10MB", split_naming="bids", verbose=True + ) + with pytest.raises(ValueError, match="Passing a BIDSPath"): + raw.save(bids_path, **save_kwargs) + bids_path.split = None + want_paths = [Path(bids_path.copy().update(split=ii).fpath) for ii in range(1, 3)] + raw.save(bids_path, **save_kwargs) + for want_path in want_paths: + assert want_path.is_file() + + def _err(*args, **kwargs): raise RuntimeError("Killed mid-write") diff --git a/mne/tests/test_epochs.py b/mne/tests/test_epochs.py index 266781b1491..fdd91fd96a0 100644 --- a/mne/tests/test_epochs.py +++ b/mne/tests/test_epochs.py @@ -1705,24 +1705,40 @@ def test_split_naming( # check that the filenames match the intended pattern assert len(list(dst_fpath.parent.iterdir())) == n_files - for i in range(n_files): - assert (tmp_path / split_fname_fn(i)).is_file() assert not (tmp_path / split_fname_fn(n_files)).is_file() + want_paths = [tmp_path / split_fname_fn(i) for i in range(n_files)] + for want_path in want_paths: + assert want_path.is_file() if not check_bids: return - # If we load sub-01_split-01_epo.fif we should then be able to write it - # without it being named sub-01_split-01_split-01_epo.fif + # gh-12451 + # If we load sub-01_split-01_epo.fif we should then we shouldn't + # write sub-01_split-01_split-01_epo.fif mne_bids = pytest.importorskip("mne_bids") # Let's try to prevent people from making a mistake bids_path = mne_bids.BIDSPath( - root=tmp_path, subject="01", split="01", suffix="epo", check=False + root=tmp_path, + subject="01", + datatype="meg", + split="01", + suffix="epo", + extension=".fif", + check=False, ) assert bids_path.fpath.is_file(), bids_path.fpath - epochs.save(bids_path, verbose=True, overwrite=True, **save_kwargs) + for want_path in want_paths: + want_path.unlink() + assert not bids_path.fpath.is_file() + with pytest.raises(ValueError, match="Passing a BIDSPath"): + epochs.save(bids_path, verbose=True, **save_kwargs) bad_path = bids_path.fpath.parent / (bids_path.fpath.stem[:-3] + "split-01_epo.fif") assert str(bad_path).count("_split-01") == 2 - assert not bad_path.is_file(), bad_path # TODO: fails! + assert not bad_path.is_file(), bad_path + bids_path.split = None + epochs.save(bids_path, verbose=True, **save_kwargs) + for want_path in want_paths: + assert want_path.is_file() @pytest.mark.parametrize( diff --git a/mne/utils/check.py b/mne/utils/check.py index 13eca1e0ba0..e73faf0a2e3 100644 --- a/mne/utils/check.py +++ b/mne/utils/check.py @@ -230,11 +230,29 @@ def _check_fname( name="File", need_dir=False, *, + check_bids_split=False, verbose=None, ): """Check for file existence, and return its absolute path.""" _validate_type(fname, "path-like", name) - fname = Path(fname).expanduser().absolute() + # special case for MNE-BIDS, check split + fname_path = Path(fname) + if check_bids_split: + try: + from mne_bids import BIDSPath + except Exception: + pass + else: + if isinstance(fname, BIDSPath) and fname.split is not None: + raise ValueError( + f"Passing a BIDSPath {name} with `{fname.split=}` is unsafe as it " + "can unexpectedly lead to invalid BIDS split naming. Explicitly " + f"set `{name}.split = None` to avoid ambiguity. If you want the " + f"old misleading split naming, you can pass `str({name})`." + ) + + fname = fname_path.expanduser().absolute() + del fname_path if fname.exists(): if not overwrite: From 4df16f14a3d80f432d354b7cbc2a35b9c621a203 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:11:23 -0500 Subject: [PATCH 03/10] FIX: no need for pytest-harvest --- mne/conftest.py | 28 +++++++++------------------- mne/datasets/__init__.pyi | 2 +- mne/datasets/_infant/__init__.py | 1 + mne/fixes.py | 5 ++--- pyproject.toml | 1 - 5 files changed, 13 insertions(+), 24 deletions(-) create mode 100644 mne/datasets/_infant/__init__.py diff --git a/mne/conftest.py b/mne/conftest.py index 17ff64b32f7..5cd45826f75 100644 --- a/mne/conftest.py +++ b/mne/conftest.py @@ -900,11 +900,8 @@ def protect_config(): def _test_passed(request): - try: - outcome = request.node.harvest_rep_call - except Exception: - outcome = "passed" - return outcome == "passed" + report = request.node.stash[_phase_report_key] + return "call" in report and report["call"].outcome == "passed" @pytest.fixture() @@ -931,7 +928,6 @@ def brain_gc(request): ignore = set(id(o) for o in gc.get_objects()) yield close_func() - # no need to warn if the test itself failed, pytest-harvest helps us here if not _test_passed(request): return _assert_no_instances(Brain, "after") @@ -960,16 +956,12 @@ def pytest_sessionfinish(session, exitstatus): if n is None: return print("\n") - try: - import pytest_harvest - except ImportError: - print("Module-level timings require pytest-harvest") - return # get the number to print - res = pytest_harvest.get_session_synthesis_dct(session) files = dict() - for key, val in res.items(): - parts = Path(key.split(":")[0]).parts + for item in session.items: + report = item.stash[_phase_report_key] + dur = sum(x.duration for x in report.values()) + parts = Path(item.nodeid.split(":")[0]).parts # split mne/tests/test_whatever.py into separate categories since these # are essentially submodule-level tests. Keeping just [:3] works, # except for mne/viz where we want level-4 granulatity @@ -978,7 +970,7 @@ def pytest_sessionfinish(session, exitstatus): if not parts[-1].endswith(".py"): parts = parts + ("",) file_key = "/".join(parts) - files[file_key] = files.get(file_key, 0) + val["pytest_duration_s"] + files[file_key] = files.get(file_key, 0) + dur files = sorted(list(files.items()), key=lambda x: x[1])[::-1] # print _files[:] = files[:n] @@ -999,7 +991,7 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config): writer.line(f"{timing.ljust(15)}{name}") -def pytest_report_header(config, startdir): +def pytest_report_header(config, startdir=None): """Add information to the pytest run header.""" return f"MNE {mne.__version__} -- {str(Path(mne.__file__).parent)}" @@ -1122,7 +1114,6 @@ def run(nbexec=nbexec, code=code): return -@pytest.mark.filterwarnings("ignore:.*Extraction of measurement.*:") @pytest.fixture( params=( [nirsport2, nirsport2_snirf, testing._pytest_param()], @@ -1160,8 +1151,7 @@ def qt_windows_closed(request): if "allow_unclosed_pyside2" in marks and API_NAME.lower() == "pyside2": return # Don't check when the test fails - report = request.node.stash[_phase_report_key] - if ("call" not in report) or report["call"].failed: + if not _test_passed(request): return widgets = app.topLevelWidgets() n_after = len(widgets) diff --git a/mne/datasets/__init__.pyi b/mne/datasets/__init__.pyi index 22cb6acce7b..44cee84fe7f 100644 --- a/mne/datasets/__init__.pyi +++ b/mne/datasets/__init__.pyi @@ -66,7 +66,7 @@ from . import ( ) from ._fetch import fetch_dataset from ._fsaverage.base import fetch_fsaverage -from ._infant.base import fetch_infant_template +from ._infant import fetch_infant_template from ._phantom.base import fetch_phantom from .utils import ( _download_all_example_data, diff --git a/mne/datasets/_infant/__init__.py b/mne/datasets/_infant/__init__.py new file mode 100644 index 00000000000..7347d36fcd0 --- /dev/null +++ b/mne/datasets/_infant/__init__.py @@ -0,0 +1 @@ +from .base import fetch_infant_template diff --git a/mne/fixes.py b/mne/fixes.py index 4759366f386..55e56261866 100644 --- a/mne/fixes.py +++ b/mne/fixes.py @@ -31,9 +31,8 @@ ############################################################################### # distutils -# distutils has been deprecated since Python 3.10 and is scheduled for removal -# from the standard library with the release of Python 3.12. For version -# comparisons, we use setuptools's `parse_version` if available. +# distutils has been deprecated since Python 3.10 and was removed +# from the standard library with the release of Python 3.12. def _compare_version(version_a, operator, version_b): diff --git a/pyproject.toml b/pyproject.toml index 49a91ba7e73..82a9423d97f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -113,7 +113,6 @@ test = [ "pytest>=8.0.0rc2", "pytest-cov", "pytest-timeout", - "pytest-harvest", "pytest-qt", "ruff", "numpydoc", From d497f900deb9603e9eb17ed306aba3e4f52a3735 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:12:27 -0500 Subject: [PATCH 04/10] FIX: Document --- doc/changes/devel/12451.dependency.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 doc/changes/devel/12451.dependency.rst diff --git a/doc/changes/devel/12451.dependency.rst b/doc/changes/devel/12451.dependency.rst new file mode 100644 index 00000000000..37906ede1f4 --- /dev/null +++ b/doc/changes/devel/12451.dependency.rst @@ -0,0 +1 @@ +``pytest-harvest`` is no longer an optional test dependency, by `Eric Larson`_. \ No newline at end of file From 151b81f17c8624a9a4ecc88ebd08b0dae88248f4 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:14:16 -0500 Subject: [PATCH 05/10] Update 12451.dependency.rst --- doc/changes/devel/12451.dependency.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/changes/devel/12451.dependency.rst b/doc/changes/devel/12451.dependency.rst index 37906ede1f4..8227dd779ad 100644 --- a/doc/changes/devel/12451.dependency.rst +++ b/doc/changes/devel/12451.dependency.rst @@ -1 +1 @@ -``pytest-harvest`` is no longer an optional test dependency, by `Eric Larson`_. \ No newline at end of file +``pytest-harvest`` is no longer used as a test dependency, by `Eric Larson`_. From a2a88d53fe30e21699c6750337e26f363beba307 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:17:12 -0500 Subject: [PATCH 06/10] FIX: might as well defaultdict --- mne/conftest.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mne/conftest.py b/mne/conftest.py index 5cd45826f75..fd7d1946843 100644 --- a/mne/conftest.py +++ b/mne/conftest.py @@ -10,6 +10,7 @@ import shutil import sys import warnings +from collections import defaultdict from contextlib import contextmanager from pathlib import Path from textwrap import dedent @@ -957,7 +958,7 @@ def pytest_sessionfinish(session, exitstatus): return print("\n") # get the number to print - files = dict() + files = defaultdict(lambda: 0.0) for item in session.items: report = item.stash[_phase_report_key] dur = sum(x.duration for x in report.values()) @@ -970,7 +971,7 @@ def pytest_sessionfinish(session, exitstatus): if not parts[-1].endswith(".py"): parts = parts + ("",) file_key = "/".join(parts) - files[file_key] = files.get(file_key, 0) + dur + files[file_key] += dur files = sorted(list(files.items()), key=lambda x: x[1])[::-1] # print _files[:] = files[:n] From 03167e48b22cf33da48166121e1722be1e12a1c6 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:19:48 -0500 Subject: [PATCH 07/10] FIX: Name --- mne/io/base.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mne/io/base.py b/mne/io/base.py index 46953f6809c..99a8e658fc4 100644 --- a/mne/io/base.py +++ b/mne/io/base.py @@ -1695,7 +1695,11 @@ def save( # convert to str, check for overwrite a few lines later fname = _check_fname( - fname, overwrite=True, verbose="error", check_bids_split=True + fname, + overwrite=True, + verbose="error", + check_bids_split=True, + name="fname", ) check_fname(fname, "raw", endings, endings_err=endings_err) From f6a98e5a1d8447a3ab45dbceb99ba7307985f2d3 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Tue, 20 Feb 2024 15:21:00 -0500 Subject: [PATCH 08/10] FIX: Explicit --- mne/io/fiff/tests/test_raw_fiff.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mne/io/fiff/tests/test_raw_fiff.py b/mne/io/fiff/tests/test_raw_fiff.py index 200a7906b8b..a28844eb5f5 100644 --- a/mne/io/fiff/tests/test_raw_fiff.py +++ b/mne/io/fiff/tests/test_raw_fiff.py @@ -696,6 +696,8 @@ def test_bids_split_files(tmp_path): raw.save(bids_path, **save_kwargs) bids_path.split = None want_paths = [Path(bids_path.copy().update(split=ii).fpath) for ii in range(1, 3)] + for want_path in want_paths: + assert not want_path.is_file() raw.save(bids_path, **save_kwargs) for want_path in want_paths: assert want_path.is_file() From b565dc8e68e25a567fb401bfb34afd6d3eeea39b Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Wed, 21 Feb 2024 08:17:28 -0500 Subject: [PATCH 09/10] Update pyproject.toml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Richard Höchenberger --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 82a9423d97f..d82518aa70e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -137,7 +137,7 @@ test_extra = [ "imageio-ffmpeg>=0.4.1", "snirf", "neo", - "mne_bids", + "mne-bids", ] # Dependencies for building the docuemntation From f993050748195ec27b3386cf7ab38b21c637e516 Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Wed, 21 Feb 2024 08:49:58 -0500 Subject: [PATCH 10/10] Update doc/changes/devel/12451.bugfix.rst Co-authored-by: Stefan Appelhoff --- doc/changes/devel/12451.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/changes/devel/12451.bugfix.rst b/doc/changes/devel/12451.bugfix.rst index 11c5c6496af..2aca44529f1 100644 --- a/doc/changes/devel/12451.bugfix.rst +++ b/doc/changes/devel/12451.bugfix.rst @@ -1 +1 @@ -Fix ambiguous and likely errant redundant use of ``BIDSPath.split`` when writing split raw and epochs data, by `Eric Larson`_. +Fix errant redundant use of ``BIDSPath.split`` when writing split raw and epochs data, by `Eric Larson`_.