From abf4738548daa21c88d74a4592ef4a5a75ef96db Mon Sep 17 00:00:00 2001 From: Stefan Appelhoff Date: Sat, 5 Aug 2023 20:52:43 +0200 Subject: [PATCH] Fix BTi handling in mne-bids (#1160) * fix bti copyfile * remove pragma, this is covered * do not rename bti files, but warn if they are weird * update whatsnew * skip tests if mne not >=1.5 * fix numbers for versions * add option to skip circle [skip circle] [skip ci] --- .circleci/config.yml | 13 ++++++++ doc/whats_new.rst | 3 +- doc/whats_new_previous_releases.rst | 2 +- mne_bids/copyfiles.py | 44 +++++++++++++++++-------- mne_bids/path.py | 2 +- mne_bids/read.py | 30 ++++++++++++++--- mne_bids/tests/test_copyfiles.py | 16 +++++++++ mne_bids/tests/test_path.py | 2 +- mne_bids/tests/test_write.py | 50 ++++++++++++++++++----------- mne_bids/write.py | 12 +++---- 10 files changed, 128 insertions(+), 46 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e1f39ce38..75b9060d4 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,6 +1,16 @@ # See: https://circleci.com/blog/deploying-documentation-to-github-pages-with-continuous-integration/ version: 2.1 +_check_skip: &check_skip + name: Check-skip + command: | + set -e + export COMMIT_MESSAGE=$(git log --format=oneline -n 1); + if [[ -v CIRCLE_PULL_REQUEST ]] && ([[ "$COMMIT_MESSAGE" == *"[skip circle]"* ]] || [[ "$COMMIT_MESSAGE" == *"[circle skip]"* ]]); then + echo "Skip detected, exiting job ${CIRCLE_JOB} for PR ${CIRCLE_PULL_REQUEST}." + circleci-agent step halt; + fi + jobs: docs-build: machine: @@ -8,6 +18,9 @@ jobs: steps: - checkout + - run: + <<: *check_skip + # restore cache from last build. Unless __init__.py has changed since then - restore_cache: keys: diff --git a/doc/whats_new.rst b/doc/whats_new.rst index df006a126..6b81600b8 100644 --- a/doc/whats_new.rst +++ b/doc/whats_new.rst @@ -50,6 +50,7 @@ Detailed list of changes - When writing events, we now also create an ``*_events.json`` file in addition to ``*_events.tsv``. This ensures compatibility with the upcoming release of BIDS 1.9, by `Richard Höchenberger`_ (:gh:`1132`) - We silenced warnings about missing ``events.tsv`` files when reading empty-room or resting-state data, by `Richard Höchenberger`_ (:gh:`1133`) +- BTi 'processed data files' (pdf) will no longer be renamed to ``c,rf*`` by default. Instead, they will be copied over without name changes. When reading BTi data via ``mne-bids``, more informative log and error messages will help choosing the right course of action, by `Stefan Appelhoff`_ (:gh:`1160`) 🛠 Requirements ^^^^^^^^^^^^^^^ @@ -65,7 +66,7 @@ Detailed list of changes - Improve compatibility with latest MNE-Python, by `Eric Larson`_ (:gh:`1128`) - Working with :class:`~mne_bids.BIDSPath` would sometimes inadvertently create new directories, contaminating the BIDS dataset, by `Richard Höchenberger`_ (:gh:`1139`) - Fix thrown error if the ``BIDSVersion`` defined in ``dataset_description.json`` file does not match the MNE-BIDS compliant ``BIDSVersion``, ensuring backwards compatibility across BIDS complient tools, by `Ford McDonald`_ (:gh:`1147`) -- Copying BTI files without a headshape file will no longer raise an error, the file will simply be copied, and the missing headshape file will be ignored, by `Stefan Appelhoff`_ (:gh:`1158`) +- Copying BTi files without a headshape file will no longer raise an error, the file will simply be copied, and the missing headshape file will be ignored, by `Stefan Appelhoff`_ (:gh:`1158`) :doc:`Find out what was new in previous releases ` diff --git a/doc/whats_new_previous_releases.rst b/doc/whats_new_previous_releases.rst index 9817eeaa4..41bee29d4 100644 --- a/doc/whats_new_previous_releases.rst +++ b/doc/whats_new_previous_releases.rst @@ -781,7 +781,7 @@ Bug fixes - Fix :func:`write_raw_bids` when ``info['dig']`` is ``None``, by `Alexandre Gramfort`_ (:gh:`452`) - :func:`mne_bids.write_raw_bids` now applies ``verbose`` to the functions that read events, by `Evgenii Kalenkovich`_ (:gh:`453`) - Fix ``raw_to_bids`` CLI tool to work with non-FIFF files, by `Austin Hurst`_ (:gh:`456`) -- Fix :func:`mne_bids.write_raw_bids` to output BTI and CTF data in the ``scans.tsv`` according to the BIDS specification, by `Adam Li`_ (:gh:`465`) +- Fix :func:`mne_bids.write_raw_bids` to output BTi and CTF data in the ``scans.tsv`` according to the BIDS specification, by `Adam Li`_ (:gh:`465`) - :func:`mne_bids.read_raw_bids` now populates the list of bad channels based on ``*_channels.tsv`` if (and only if) a ``status`` column is present, ignoring similar metadata stored in raw file (which will still be used if **no** ``status`` column is present in ``*_channels.tsv``), by `Richard Höchenberger`_ (:gh:`499`) - Ensure that ``Raw.info['bads']`` returned by :func:`mne_bids.read_raw_bids` is always a list, by `Richard Höchenberger`_ (:gh:`501`) - :func:`mne_bids.write_raw_bids` now ensures that **all** parts of the :class:`mne.io.Raw` instance stay in sync when using anonymization to shift dates, e.g. ``raw.annotations``, by `Richard Höchenberger`_ (:gh:`504`) diff --git a/mne_bids/copyfiles.py b/mne_bids/copyfiles.py index 81542e22e..acc5fe8e9 100644 --- a/mne_bids/copyfiles.py +++ b/mne_bids/copyfiles.py @@ -16,18 +16,19 @@ # License: BSD-3-Clause import os import os.path as op +from pathlib import Path import re import shutil as sh -from pathlib import Path - -from scipy.io import loadmat, savemat +import mne from mne.io import read_raw_brainvision, read_raw_edf, read_raw_bdf, anonymize_info from mne.utils import logger, verbose +from mne.fixes import _compare_version +import numpy as np +from scipy.io import loadmat, savemat from mne_bids.path import BIDSPath, _parse_ext, _mkdir_p from mne_bids.utils import _get_mrk_meas_date, _check_anonymize, warn -import numpy as np def _copytree(src, dst, **kwargs): @@ -616,13 +617,28 @@ def copyfile_bti(raw, dest): copyfile_kit """ - pdf_fname = "c,rfDC" - if raw.info["highpass"] is not None: - pdf_fname = "c,rf%0.1fHz" % raw.info["highpass"] - sh.copyfile(raw._init_kwargs["pdf_fname"], op.join(dest, pdf_fname)) - sh.copyfile(raw._init_kwargs["config_fname"], op.join(dest, "config")) - - # If no headshape file present, cannot copy it - hs_file = raw._init_kwargs.get("head_shape_fname") - if hs_file is not None: # pragma: no cover - sh.copyfile(hs_file, op.join(dest, "hs_file")) + # XXX: remove after support for mne<1.5 is dropped + # BTi operations were unreliable in mne-bids prior to mne 1.5 / mne-bids 0.13 + need_mne = "1.4.2" + if not _compare_version(mne.__version__, ">", need_mne): # pragma: no cover + raise ImportError( + f"mne > {need_mne} is required for BTi operations, got {mne.__version__}" + ) + + os.makedirs(dest, exist_ok=True) + for key, val in ( + ("pdf_fname", None), + ("config_fname", "config"), + ("head_shape_fname", "hs_file"), + ): + keyfile = raw._raw_extras[0].get(key) + + # Keep name of pdf file + if key == "pdf_fname": + val = op.basename(keyfile) + + # If no headshape file present, cannot copy it + if key == "head_shape_fname" and keyfile is None: + continue + + sh.copyfile(keyfile, op.join(dest, val)) diff --git a/mne_bids/path.py b/mne_bids/path.py index a9a862576..7d9e1b02a 100644 --- a/mne_bids/path.py +++ b/mne_bids/path.py @@ -1320,7 +1320,7 @@ def _get_matching_bidspaths_from_filesystem(bids_path): subject=sub, session=ses, datatype=datatype, root=bids_root ).directory - # For BTI data, just return the directory with a '.pdf' extension + # For BTi data, just return the directory with a '.pdf' extension # to facilitate reading in mne-bids bti_dir = op.join(data_dir, f"{basename}") if op.isdir(bti_dir): diff --git a/mne_bids/read.py b/mne_bids/read.py index d20563c51..624183b9c 100644 --- a/mne_bids/read.py +++ b/mne_bids/read.py @@ -61,8 +61,8 @@ def _read_raw( # BTi systems elif ext == ".pdf": raw = io.read_raw_bti( - pdf_fname=str(raw_path), # FIXME MNE should accept Path! - config_fname=str(config_path), # FIXME MNE should accept Path! + pdf_fname=raw_path, + config_fname=config_path, head_shape_fname=hsp, preload=False, **kwargs, @@ -280,7 +280,7 @@ def _handle_scans_reading(scans_fname, raw, bids_path): fname = bids_path.fpath.name if fname.endswith(".pdf"): - # for BTI files, the scan is an entire directory + # for BTi files, the scan is an entire directory fname = fname.split(".")[0] # get the row corresponding to the file @@ -759,7 +759,29 @@ def read_raw_bids(bids_path, extra_params=None, verbose=None): if bids_path.fpath.suffix == ".pdf": bids_raw_folder = bids_path.directory / f"{bids_path.basename}" - raw_path = list(bids_raw_folder.glob("c,rf*"))[0] + + # try to find the processed data file ("pdf") + # see: https://www.fieldtriptoolbox.org/getting_started/bti/ + bti_pdf_patterns = ["0", "c,rf*", "hc,rf*", "e,rf*"] + pdf_list = [] + for pattern in bti_pdf_patterns: + pdf_list += sorted(bids_raw_folder.glob(pattern)) + + if len(pdf_list) == 0: + raise RuntimeError( + "Cannot find BTi 'processed data file' (pdf). Please open an issue on " + "the mne-bids repository to discuss with the developers:\n\n" + "https://github.com/mne-tools/mne-bids/issues/new/choose\n\n" + f"No matches for following patterns:\n\n{bti_pdf_patterns}\n\n" + f"In: {bids_raw_folder}" + ) + elif len(pdf_list) > 1: # pragma: no cover + logger.warn( + "Found more than one BTi 'processed data file' (pdf). " + f"Picking:\n\n {pdf_list[0]}\n\nout of the options:\n\n" + f"{pdf_list}\n\n" + ) + raw_path = pdf_list[0] config_path = bids_raw_folder / "config" else: raw_path = bids_path.fpath diff --git a/mne_bids/tests/test_copyfiles.py b/mne_bids/tests/test_copyfiles.py index 5b37c08c0..2a3cc58de 100644 --- a/mne_bids/tests/test_copyfiles.py +++ b/mne_bids/tests/test_copyfiles.py @@ -21,6 +21,7 @@ copyfile_edf, copyfile_eeglab, copyfile_kit, + copyfile_bti, ) testing_path = testing.data_path(download=False) @@ -299,3 +300,18 @@ def test_copyfile_kit(tmp_path): root=output_path, ) assert op.exists(hsp_fname) + + +@pytest.mark.parametrize("dataset", ("4Dsim", "erm_HFH")) +@testing.requires_testing_data +def test_copyfile_bti(tmp_path, dataset): + """Test copying and renaming BTi files to a new location.""" + pytest.importorskip("mne", "1.5.0.dev") # XXX: remove when mne<1.5 is dropped + pdf_fname = testing_path / "BTi" / dataset / "c,rfDC" + kwargs = dict(head_shape_fname=None) if dataset == "erm_HFH" else dict() + raw = mne.io.read_raw_bti(pdf_fname, **kwargs, verbose=False) + + dest = tmp_path / dataset + copyfile_bti(raw=raw, dest=dest) + + mne.io.read_raw_bti(dest / "c,rfDC", **kwargs, verbose=False) diff --git a/mne_bids/tests/test_path.py b/mne_bids/tests/test_path.py index 0f49f603e..4d5780e3d 100644 --- a/mne_bids/tests/test_path.py +++ b/mne_bids/tests/test_path.py @@ -332,7 +332,7 @@ def test_parse_ext(): assert fname == "sub-05_task-matchingpennies" assert ext == ".vhdr" - # Test for case where no extension: assume BTI format + # Test for case where no extension: assume BTi format f = "sub-01_task-rest" fname, ext = _parse_ext(f) assert fname == f diff --git a/mne_bids/tests/test_write.py b/mne_bids/tests/test_write.py index 63ad528e8..f0437b31f 100644 --- a/mne_bids/tests/test_write.py +++ b/mne_bids/tests/test_write.py @@ -1187,17 +1187,27 @@ def test_ctf(_bids_validate, tmp_path): get_anonymization_daysback(raw) +@pytest.mark.parametrize("dataset", ("inbuilt_linux", "4Dsim", "erm_HFH")) +@testing.requires_testing_data @pytest.mark.filterwarnings(warning_str["channel_unit_changed"]) -def test_bti(_bids_validate, tmp_path): +def test_bti(_bids_validate, tmp_path, dataset): """Test functionality of the write_raw_bids conversion for BTi data.""" - bti_path = op.join(base_path, "bti", "tests", "data") - raw_fname = op.join(bti_path, "test_pdf_linux") - config_fname = op.join(bti_path, "test_config_linux") - headshape_fname = op.join(bti_path, "test_hs_linux") + pytest.importorskip("mne", "1.5.0.dev") # XXX: remove when mne<1.5 is dropped + if dataset == "inbuilt_linux": + bti_path = op.join(base_path, "bti", "tests", "data") + pdf_fname = op.join(bti_path, "test_pdf_linux") + kwargs = dict( + config_fname=op.join(bti_path, "test_config_linux"), + head_shape_fname=op.join(bti_path, "test_hs_linux"), + ) + elif dataset == "4Dsim": + pdf_fname = data_path / "BTi" / dataset / "c,rfDC" + kwargs = dict() + else: + pdf_fname = data_path / "BTi" / dataset / "c,rfDC" + kwargs = dict(head_shape_fname=None) - raw = _read_raw_bti( - raw_fname, config_fname=config_fname, head_shape_fname=headshape_fname - ) + raw = _read_raw_bti(pdf_fname, **kwargs) bids_path = _bids_path.copy().update(root=tmp_path, datatype="meg") @@ -1216,18 +1226,22 @@ def test_bti(_bids_validate, tmp_path): assert op.exists(tmp_path / "participants.tsv") _bids_validate(tmp_path) - raw = read_raw_bids(bids_path=bids_path) + if dataset == "inbuilt_linux": + # Reading this is impossible, because the pdf file was renamed + # to some idiosyncratic name + with pytest.raises(RuntimeError, match="Cannot find BTi .*"): + raw = read_raw_bids(bids_path=bids_path) - with pytest.raises(TypeError, match="unexpected keyword argument 'foo'"): - read_raw_bids(bids_path=bids_path, extra_params=dict(foo="bar")) + # also test anonymize + raw = _read_raw_bti(pdf_fname, **kwargs) + with pytest.warns(RuntimeWarning, match="Converting to FIF for anonymization"): + output_path = _test_anonymize(tmp_path / "tmp", raw, bids_path) + _bids_validate(output_path) - # test anonymize - raw = _read_raw_bti( - raw_fname, config_fname=config_fname, head_shape_fname=headshape_fname - ) - with pytest.warns(RuntimeWarning, match="Converting to FIF for anonymization"): - output_path = _test_anonymize(tmp_path / "tmp", raw, bids_path) - _bids_validate(output_path) + else: + # The other datasets can be read, as their pdf file names were + # not changed and still fit the "standard" naming patterns + raw = read_raw_bids(bids_path=bids_path) @pytest.mark.filterwarnings( diff --git a/mne_bids/write.py b/mne_bids/write.py index 044e56455..3cb8be73c 100644 --- a/mne_bids/write.py +++ b/mne_bids/write.py @@ -877,9 +877,9 @@ def _sidecar_json( FIFF.FIFFV_POINT_LPA, ]: digitized_landmark = True - elif dig_point["kind"] == FIFF.FIFFV_POINT_EXTRA and raw.filenames[ - 0 - ].endswith(".fif"): + elif dig_point["kind"] == FIFF.FIFFV_POINT_EXTRA and str( + raw.filenames[0] + ).endswith(".fif"): digitized_head_points = True software_filters = { "SpatialCompensation": {"GradientOrder": raw.compensation_grade} @@ -1685,9 +1685,9 @@ def write_raw_bids( if ".ds" in op.dirname(raw.filenames[0]): raw_fname = op.dirname(raw.filenames[0]) # point to file containing header info for multifile systems - raw_fname = raw_fname.replace(".eeg", ".vhdr") - raw_fname = raw_fname.replace(".fdt", ".set") - raw_fname = raw_fname.replace(".dat", ".lay") + raw_fname = str(raw_fname).replace(".eeg", ".vhdr") + raw_fname = str(raw_fname).replace(".fdt", ".set") + raw_fname = str(raw_fname).replace(".dat", ".lay") _, ext = _parse_ext(raw_fname) # force all EDF/BDF files with upper-case extension to be written as