Fix Schirrmeister2017 re-downloading every recording

docs/source/whats_new.rst

@@ -57,6 +57,7 @@ Requirements
 
 Bugs
 ~~~~
+- Fix :class:`moabb.datasets.Schirrmeister2017` re-downloading every recording. ``data_path`` moved each freshly fetched EDF out of the directory that :func:`moabb.datasets.download.data_dl` owns and into ``MNE-schirrmeister2017-data/<train|test>/``, so the next call found the download cache empty and fetched the whole file again. The refetched copy was then left behind because the destination already existed, leaving two copies of a multi-gigabyte recording on disk. ``data_path`` now returns the path :func:`~moabb.datasets.download.data_dl` reports and only reads from the old location when a file is already there, so an existing local copy is still reused and never re-downloaded (:gh:`851` by `Aditya Singh`_)
 - Fix how the benchmark results page (:doc:`paper_results`) describes what its tables report. It stated that results are "mean accuracy and standard deviation across all folds for all sessions and subjects", and both halves are inaccurate: :class:`moabb.paradigms.MotorImagery` selects the metric from the number of classes, so two-class scenarios are scored with ROC-AUC rather than accuracy, and :class:`moabb.evaluations.WithinSessionEvaluation` averages the cross-validation folds within each session before returning a score, so the reported standard deviation is across (subject, session) pairs and not across individual folds (:gh:`1128` by `Bhargav Kowshik`_)
 - Fix datasets ignoring a change of download directory: datasets now inherit ``MNE_DATA`` without persisting a redundant per-dataset mirror, and :func:`moabb.utils.set_download_dir` removes legacy ``MNE_DATASETS_<SIGN>_PATH`` entries that still mirror the previous shared location while preserving explicit overrides. :class:`moabb.datasets.RomaniBF2025ERP` now uses the same path mechanism and honours ``path`` and ``force_update``. Adds isolated regression coverage across every dataset (:gh:`1115` by `Bruno Aristimunha`_).
 - Add a ``__repr__`` to :class:`moabb.datasets.base.BaseDataset` so datasets display by their code (e.g. ``BNCI2014-001``) when printed, instead of the verbose default ``<...object at 0x...>``. This declutters the output of ``print(paradigm.datasets)`` in the tutorials and of the paradigm and evaluation compatibility warnings (by `Danae`_)
@@ -923,4 +924,5 @@ API changes
 .. _Danae: https://github.com/dnplchrn
 .. _Henrique Lefundes: https://github.com/HenriqueLefundes
 .. _Paul-Adrien Graignic: https://github.com/pagraignic-yneuro
+.. _Aditya Singh: https://github.com/adityasingh2400
 .. _pre-commit-ci: https://github.com/apps/pre-commit-ci

moabb/datasets/schirrmeister2017.py

@@ -1,6 +1,5 @@
 import logging
 import os
-import shutil
 
 import mne
 from mne.channels import make_standard_montage
@@ -381,29 +380,25 @@ def _url(prefix):
         base_path = dl.get_dataset_path("SCHIRRMEISTER2017", path)
         dataset_folder = os.path.join(base_path, "MNE-schirrmeister2017-data")
 
-        # Create subfolder paths
         paths = []
         for t in ["train", "test"]:
-            url = _url(t)
-            # Extract subfolder name from URL
-            subfolder = t
+            # Earlier versions of this loader moved the downloaded file out of
+            # the location owned by ``data_dl`` into ``<dataset_folder>/<t>/``.
+            # Keep reading from there so an existing local copy is not
+            # downloaded again, but never write to it: moving the file away
+            # makes ``data_dl`` miss its own cache and re-fetch the recording.
+            legacy_path = os.path.join(dataset_folder, t, "{:d}.edf".format(subject))
+            if os.path.isfile(legacy_path):
+                if not force_update:
+                    paths.append(legacy_path)
+                    continue
+                # A refreshed copy is written to the canonical location, so
+                # drop the stale one instead of letting it shadow the new file.
+                os.remove(legacy_path)
 
-            # Download the file to a temporary location
-            temp_path = dl.data_dl(url, "SCHIRRMEISTER2017", path, force_update, verbose)
-
-            # Create the proper subfolder structure
-            subfolder_path = os.path.join(dataset_folder, subfolder)
-            os.makedirs(subfolder_path, exist_ok=True)
-
-            # Move file to the correct subfolder
-            filename = os.path.basename(temp_path)
-            new_path = os.path.join(subfolder_path, filename)
-
-            # If file already exists in target location, no need to move it
-            if not os.path.exists(new_path):
-                shutil.move(temp_path, new_path)
-
-            paths.append(new_path)
+            paths.append(
+                dl.data_dl(_url(t), "SCHIRRMEISTER2017", path, force_update, verbose)
+            )
 
         return paths
 

moabb/tests/test_dataset_fixes.py

@@ -7,9 +7,12 @@
 from unittest.mock import patch
 
 import numpy as np
+import pytest
 
 from moabb.datasets import download as _dl
+from moabb.datasets import schirrmeister2017
 from moabb.datasets.bnci.bnci_2020 import _convert_attention_shift
+from moabb.datasets.schirrmeister2017 import Schirrmeister2017
 from moabb.datasets.ssvep_mamem import MAMEM1
 
 
@@ -121,3 +124,62 @@ def test_mamem_already_downloaded_does_not_ping_figshare(tmp_path: Path):
             assert paths
             assert all(Path(p).exists() for p in paths)
     _dl.fs_get_file_list.cache_clear()
+
+
+def _recording_data_dl(calls):
+    """Stand-in for ``data_dl`` that records every actual download.
+
+    It reproduces the caching contract of the real function: the file is
+    written to the location derived from the URL and reused on later calls.
+    """
+
+    def data_dl(url, sign, path=None, force_update=False, verbose=None):
+        root = Path(_dl.get_dataset_path(sign, path)) / f"MNE-{sign.lower()}-data"
+        destination = _dl._sanitize_path(_dl._normalize_destination(url, root))
+        if destination.is_file() and not force_update:
+            return str(destination)
+        calls.append(url)
+        destination.parent.mkdir(parents=True, exist_ok=True)
+        destination.write_bytes(b"edf")
+        return str(destination)
+
+    return data_dl
+
+
+def test_schirrmeister2017_does_not_redownload(tmp_path: Path, monkeypatch):
+    """``data_path`` must fetch each recording only once (issue #851)."""
+    monkeypatch.setenv("MNE_DATA", str(tmp_path))
+    calls = []
+    monkeypatch.setattr(schirrmeister2017.dl, "data_dl", _recording_data_dl(calls))
+
+    dataset = Schirrmeister2017()
+    first = dataset.data_path(1, path=str(tmp_path))
+    assert len(calls) == 2
+
+    for _ in range(2):
+        assert dataset.data_path(1, path=str(tmp_path)) == first
+    assert len(calls) == 2, "the recordings were downloaded more than once"
+
+    # A single copy of each recording, not one per storage layout.
+    assert len(list(tmp_path.rglob("1.edf"))) == 2
+
+
+def test_schirrmeister2017_reuses_relocated_files(tmp_path: Path, monkeypatch):
+    """A copy left by the old layout is reused instead of downloaded again."""
+    monkeypatch.setenv("MNE_DATA", str(tmp_path))
+    dataset_folder = tmp_path / "MNE-schirrmeister2017-data"
+    relocated = []
+    for split in ("train", "test"):
+        path = dataset_folder / split / "1.edf"
+        path.parent.mkdir(parents=True, exist_ok=True)
+        path.write_bytes(b"edf")
+        relocated.append(str(path))
+
+    monkeypatch.setattr(
+        schirrmeister2017.dl,
+        "data_dl",
+        lambda *args, **kwargs: pytest.fail("downloaded an already available file"),
+    )
+
+    dataset = Schirrmeister2017()
+    assert dataset.data_path(1, path=str(tmp_path)) == relocated