Skip to content

Commit

Permalink
[MRG] Raise an error message when src==dest in write_raw_bids (#889)
Browse files Browse the repository at this point in the history
* Fixing unit tests

* some cleanup

* fix tests

* remove unneeded note

* add whatsnew

* bump cache

* make mne 1.0 a minimum requirement

* flake

* doc: update links, move ref section

* remove mne 1.0 snirf checks

* flake and windows fix

* rename BIDSPath to BIDS path in BIDSPath docstr

* use FileExistsError and black indentation

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
  • Loading branch information
adam2392 and sappelhoff authored Aug 4, 2022
1 parent 213c378 commit be6bd62
Show file tree
Hide file tree
Showing 16 changed files with 89 additions and 104 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/unit_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
- uses: actions/cache@v3
with:
path: ${{ env.pythonLocation }}
key: build-0-${{ env.pythonLocation }}-${{ hashFiles('setup.cfg') }}-${{ hashFiles('test_requirements.txt') }}
key: build-1-${{ env.pythonLocation }}-${{ hashFiles('setup.cfg') }}-${{ hashFiles('test_requirements.txt') }}

- name: Install dependencies
run: |
Expand Down Expand Up @@ -174,7 +174,7 @@ jobs:
- name: Install MNE (stable)
if: "matrix.mne-version == 'mne-stable'"
run: |
git clone --depth 1 https://github.com/mne-tools/mne-python.git -b maint/0.24
git clone --depth 1 https://github.com/mne-tools/mne-python.git -b maint/1.0
python -m pip install -e ./mne-python
- name: Install MNE (main)
Expand Down
2 changes: 1 addition & 1 deletion doc/install.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Dependencies

Required:

* ``mne`` (>=0.24)
* ``mne`` (>=1.0)
* ``numpy`` (>=1.16.0)
* ``scipy`` (>=1.2.0, or >=1.5.0 for certain operations with EEGLAB data)
* ``setuptools`` (>=46.4.0)
Expand Down
4 changes: 4 additions & 0 deletions doc/whats_new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ Detailed list of changes
🛠 Requirements
^^^^^^^^^^^^^^^

- MNE-BIDS now requires MNE-Python 1.0 or newer.

- Writing BrainVision files now requires ``pybv`` version ``0.7.3``, by `Stefan Appelhoff`_ (:gh:`1011`)

🪲 Bug fixes
Expand All @@ -84,6 +86,8 @@ Detailed list of changes

- Writing and copying CTF files now works on Windows when files already exist (``overwrite=True``), by `Stefan Appelhoff`_ (:gh:`1035`)

- Instead of deleting files and raising cryptic errors, an intentional error message is now sent when calling :func:`~mne_bids.write_raw_bids` with the source file identical to the destination file, unless ``format`` is specified, by `Adam Li`_ and `Stefan Appelhoff`_ (:gh:`889`)

:doc:`Find out what was new in previous releases <whats_new_previous_releases>`

.. include:: authors.rst
2 changes: 1 addition & 1 deletion examples/convert_eeg_to_bids.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
.. currentmodule:: mne_bids
.. _BrainVision format: https://www.brainproducts.com/productdetails.php?id=21&tab=5
.. _BrainVision format: https://www.brainproducts.com/support-resources/brainvision-core-data-format-1-0/
.. _CapTrak: https://www.fieldtriptoolbox.org/faq/coordsys/#details-of-the-captrak-coordinate-system
""" # noqa: E501
Expand Down
15 changes: 8 additions & 7 deletions examples/rename_brainvision_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,7 @@
For the command line version of this tool, see the :code:`cp` tool in the docs
for the :ref:`Python Command Line Interface <python_cli>`.
References
----------
.. [1] Pernet, C.R., Appelhoff, S., Gorgolewski, K.J. et al. EEG-BIDS, an
extension to the brain imaging data structure for
electroencephalography. Sci Data 6, 103 (2019).
https://doi.org/10.1038/s41597-019-0104-8
.. _BrainVision data format: https://www.brainproducts.com/productdetails.php?id=21&tab=5
.. _BrainVision data format: https://www.brainproducts.com/support-resources/brainvision-core-data-format-1-0/
""" # noqa:E501

# Authors: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Expand Down Expand Up @@ -100,3 +94,10 @@
#
# .. _`pybv`: https://github.com/bids-standard/pybv
# .. _`bv-validator`: https://github.com/sappelhoff/brainvision-validator
#
# References
# ----------
# .. [1] Pernet, C.R., Appelhoff, S., Gorgolewski, K.J. et al. EEG-BIDS, an
# extension to the brain imaging data structure for
# electroencephalography. Sci Data 6, 103 (2019).
# https://doi.org/10.1038/s41597-019-0104-8
2 changes: 1 addition & 1 deletion mne_bids/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@
['.mrk'] # KIT/Yokogawa/Ricoh marker coil
)

# allowed BIDS path entities
# allowed BIDSPath entities
ALLOWED_PATH_ENTITIES = ('subject', 'session', 'task', 'run',
'processing', 'recording', 'space',
'acquisition', 'split',
Expand Down
8 changes: 4 additions & 4 deletions mne_bids/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ def fpath(self):
# else, return the relative path with the basename
if (self.suffix is None or self.extension is None) and \
self.root is not None:
# get matching BIDS paths inside the bids root
# get matching BIDSPaths inside the bids root
matching_paths = \
_get_matching_bidspaths_from_filesystem(self)

Expand Down Expand Up @@ -645,7 +645,7 @@ def update(self, *, check=None, **kwargs):
the existing ``.check`` attribute instead, which is set upon
`mne_bids.BIDSPath` instantiation. Defaults to ``None``.
**kwargs : dict
It can contain updates for valid BIDS path entities:
It can contain updates for valid BIDSPath entities:
'subject', 'session', 'task', 'acquisition', 'processing', 'run',
'recording', 'space', 'suffix', 'split', 'extension',
or updates for 'root' or 'datatype'.
Expand Down Expand Up @@ -981,7 +981,7 @@ def meg_crosstalk_fpath(self):


def _get_matching_bidspaths_from_filesystem(bids_path):
"""Get matching file paths for a BIDS path.
"""Get matching file paths for a BIDSPath.
Assumes suffix and/or extension is not provided.
"""
Expand Down Expand Up @@ -1264,7 +1264,7 @@ def get_bids_path_from_fname(fname, check=True, verbose=None):
Returns
-------
bids_path : BIDSPath
The BIDS path object.
The BIDSPath object.
"""
fpath = Path(fname)
fname = fpath.name
Expand Down
22 changes: 2 additions & 20 deletions mne_bids/read.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@
from pathlib import Path
import json
import re
import warnings
from datetime import datetime, timezone

import numpy as np
import mne
from mne import io, read_events, events_from_annotations
from mne.io.pick import pick_channels_regexp
from mne.utils import (
has_nibabel, logger, warn, get_subjects_dir, check_version
has_nibabel, logger, warn, get_subjects_dir
)
from mne.coreg import fit_matched_points
from mne.transforms import apply_trans
Expand Down Expand Up @@ -57,13 +56,6 @@ def _read_raw(raw_path, electrode=None, hsp=None, hpi=None,
raw = reader[ext](raw_path, allow_maxshield, **kwargs)

elif ext in ['.ds', '.vhdr', '.set', '.edf', '.bdf', '.EDF', '.snirf']:
if (
ext == '.snirf' and
not check_version('mne', '1.0')
): # pragma: no cover
raise RuntimeError(
'fNIRS support in MNE-BIDS requires MNE-Python version 1.0'
)
raw_path = Path(raw_path)
raw = reader[ext](raw_path, **kwargs)

Expand Down Expand Up @@ -286,17 +278,7 @@ def _handle_scans_reading(scans_fname, raw, bids_path):
# by the MNE documentation, and in fact we cannot load e.g. OpenNeuro
# ds003392 without this combination.
raw.set_meas_date(None)
with warnings.catch_warnings():
# This is to silence a warning emitted by MNE-Python < 0.24. The
# warnings filter can be safely removed once we drop support for
# MNE-Python 0.23 and older.
warnings.filterwarnings(
action='ignore',
message="Input info has 'meas_date' set to None",
category=RuntimeWarning,
module='mne'
)
raw.anonymize(daysback=None, keep_his=True)
raw.anonymize(daysback=None, keep_his=True)
raw.set_meas_date(acq_time)

return raw
Expand Down
4 changes: 2 additions & 2 deletions mne_bids/report/_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ def _summarize_sidecar_json(root, scans_fpaths):

n_scans += 1

# convert to BIDS Path
# convert to BIDSPath
if not isinstance(bids_path, BIDSPath):
bids_path = get_bids_path_from_fname(bids_path)
bids_path.root = root
Expand Down Expand Up @@ -401,7 +401,7 @@ def _summarize_channels_tsv(root, scans_fpaths):
if datatype not in ['meg', 'eeg', 'ieeg']:
continue

# convert to BIDS Path
# convert to BIDSPath
if not isinstance(bids_path, BIDSPath):
bids_path = get_bids_path_from_fname(bids_path)
bids_path.root = root
Expand Down
11 changes: 5 additions & 6 deletions mne_bids/tests/test_path.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Test for the MNE BIDS path functions."""
"""Test for the MNE BIDSPath functions."""
# Authors: Adam Li <adam2392@gmail.com>
#
# License: BSD-3-Clause
Expand Down Expand Up @@ -490,7 +490,7 @@ def test_bids_path(return_bids_test_dir):
f'ses-{session_id}', 'meg')
assert str(bids_path.fpath.parent) == expected_parent_dir

# test bids path without bids_root, suffix, extension
# test BIDSPath without bids_root, suffix, extension
# basename and fpath should be the same
expected_basename = f'sub-{subject_id}_ses-{session_id}_task-{task}_run-{run}' # noqa
assert (op.basename(bids_path.fpath) ==
Expand Down Expand Up @@ -853,8 +853,6 @@ def test_find_empty_room(return_bids_test_dir, tmp_path):
'sample_audvis_trunc_raw.fif')
bids_root = tmp_path / "bids"
bids_root.mkdir()
tmp_dir = tmp_path / "tmp"
tmp_dir.mkdir()

raw = _read_raw_fif(raw_fname)
bids_path = BIDSPath(subject='01', session='01',
Expand All @@ -870,6 +868,8 @@ def test_find_empty_room(return_bids_test_dir, tmp_path):
# The testing data has no "noise" recording, so save the actual data
# as named as if it were noise. We first need to write the FIFF file
# before reading it back in.
tmp_dir = tmp_path / "tmp"
tmp_dir.mkdir()
er_raw_fname = op.join(tmp_dir, 'ernoise_raw.fif')
raw.copy().crop(0, 10).save(er_raw_fname, overwrite=True)
er_raw = _read_raw_fif(er_raw_fname)
Expand Down Expand Up @@ -910,13 +910,12 @@ def test_find_empty_room(return_bids_test_dir, tmp_path):
raw = read_raw_bids(bids_path=bids_path)
raw.set_meas_date(None)
anonymize_info(raw.info)
write_raw_bids(raw, bids_path, overwrite=True)
write_raw_bids(raw, bids_path, overwrite=True, format="FIF")
with pytest.raises(ValueError, match='The provided recording does not '
'have a measurement date set'):
bids_path.find_empty_room()

# test that the `AssociatedEmptyRoom` key in MEG sidecar is respected

bids_root = tmp_path / 'associated-empty-room'
bids_root.mkdir()
raw = _read_raw_fif(raw_fname)
Expand Down
6 changes: 3 additions & 3 deletions mne_bids/tests/test_read.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ def test_handle_info_reading(tmp_path):

# setting line_freq to None should produce 'n/a' in the JSON sidecar
raw.info['line_freq'] = None
write_raw_bids(raw, bids_path, overwrite=True)
write_raw_bids(raw, bids_path, overwrite=True, format="FIF")
raw = read_raw_bids(bids_path=bids_path)
assert raw.info['line_freq'] is None

Expand All @@ -605,7 +605,7 @@ def test_handle_info_reading(tmp_path):
# 2. if line frequency is not set in raw file, then ValueError
del raw.info['line_freq']
with pytest.raises(ValueError, match="PowerLineFrequency .* required"):
write_raw_bids(raw, bids_path, overwrite=True)
write_raw_bids(raw, bids_path, overwrite=True, format="FIF")

# check whether there are "Extra points" in raw.info['dig'] if
# DigitizedHeadPoints is set to True and not otherwise
Expand Down Expand Up @@ -636,7 +636,7 @@ def test_handle_info_reading(tmp_path):
# in addition, it should not break the sidecar reading
# in `read_raw_bids`
raw.info['line_freq'] = 60
write_raw_bids(raw, bids_path, overwrite=True)
write_raw_bids(raw, bids_path, overwrite=True, format="FIF")
deriv_dir = tmp_path / 'derivatives'
deriv_dir.mkdir()
sidecar_copy = deriv_dir / op.basename(sidecar_fname)
Expand Down
42 changes: 27 additions & 15 deletions mne_bids/tests/test_write.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,10 +539,7 @@ def test_fif(_bids_validate, tmp_path):
assert op.isfile(op.join(bids_dir, sidecar_basename.basename))

bids_path.update(root=bids_root, datatype='eeg')
if check_version('mne', '0.24'):
with pytest.warns(RuntimeWarning, match='Not setting position'):
raw2 = read_raw_bids(bids_path=bids_path)
else:
with pytest.warns(RuntimeWarning, match='Not setting position'):
raw2 = read_raw_bids(bids_path=bids_path)
os.remove(op.join(bids_root, 'test-raw.fif'))

Expand Down Expand Up @@ -1216,7 +1213,7 @@ def test_eegieeg(dir_name, fname, reader, _bids_validate, tmp_path):
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')

raw = reader(raw_fname)
Expand Down Expand Up @@ -1724,10 +1721,6 @@ def test_eegieeg(dir_name, fname, reader, _bids_validate, tmp_path):
os.environ.get('BIDS_VALIDATOR_BRANCH') != 'NIRS',
reason="requires Rob's NIRS branch of bids-validator"
)
@pytest.mark.skipif(
not check_version('mne', '1.0'),
reason='requires MNE-Python 1.0'
)
def test_snirf(_bids_validate, tmp_path):
"""Test write_raw_bids conversion for SNIRF data."""
raw_fname = op.join(testing.data_path(), 'SNIRF', 'MNE-NIRS', '20220217',
Expand Down Expand Up @@ -2751,7 +2744,7 @@ def test_coordsystem_json_compliance(
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root,
datatype=datatype)

Expand Down Expand Up @@ -3067,7 +3060,7 @@ def test_convert_eeg_formats(dir_name, format, fname, reader, tmp_path):
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')

raw = reader(raw_fname)
Expand Down Expand Up @@ -3144,7 +3137,7 @@ def test_format_conversion_overwrite(dir_name, format, fname, reader,
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')

raw = reader(raw_fname)
Expand Down Expand Up @@ -3200,7 +3193,7 @@ def test_convert_meg_formats(dir_name, format, fname, reader, tmp_path):
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root, datatype='meg')

raw = reader(raw_fname)
Expand Down Expand Up @@ -3239,7 +3232,7 @@ def test_convert_raw_errors(dir_name, fname, reader, tmp_path):
data_path = op.join(testing.data_path(), dir_name)
raw_fname = op.join(data_path, fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')

# test conversion to BrainVision/FIF
Expand Down Expand Up @@ -3300,7 +3293,7 @@ def test_write_extension_case_insensitive(_bids_validate, tmp_path, datatype):
new_raw_fname = op.join(data_path, new_fname)
os.rename(raw_fname, new_raw_fname)

# the BIDS path for test datasets to get written to
# the BIDSPath for test datasets to get written to
raw = reader(new_raw_fname)
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')
write_raw_bids(raw, bids_path)
Expand Down Expand Up @@ -3764,3 +3757,22 @@ def test_anonymize_dataset_daysback(tmpdir):
rng=np.random.default_rng(),
show_progress_thresh=20
)


def test_repeat_write_location(tmpdir):
"""Test error writing BIDS dataset to the same location."""
# Get test data
raw_fname = testing.data_path() / "EDF" / "test_reduced.edf"
raw = _read_raw_edf(raw_fname)

# Write as BIDS
bids_root = tmpdir.mkdir('bids2')
bids_path = _bids_path.copy().update(root=bids_root)
bids_path = write_raw_bids(raw, bids_path, verbose=False)

# Read back in
raw = read_raw_bids(bids_path, verbose=False)

# Re-writing with src == dest should error
with pytest.raises(FileExistsError, match='Desired output BIDSPath'):
write_raw_bids(raw, bids_path, overwrite=True, verbose=False)
6 changes: 1 addition & 5 deletions mne_bids/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from mne.channels import make_standard_montage
from mne.io.kit.kit import get_kit_info
from mne.io.pick import pick_types
from mne.utils import warn, logger, verbose, check_version
from mne.utils import warn, logger, verbose

from mne_bids.tsv_handler import _to_tsv

Expand Down Expand Up @@ -128,10 +128,6 @@ def _handle_datatype(raw, datatype):
if 'eeg' in raw:
datatypes.append('eeg')
if 'fnirs_cw_amplitude' in raw:
if not check_version('mne', '1.0'): # pragma: no cover
raise RuntimeError(
'fNIRS support in MNE-BIDS requires MNE-Python version 1.0'
)
datatypes.append('nirs')
if len(datatypes) == 0:
raise ValueError('No MEG, EEG or iEEG channels found in data. '
Expand Down
Loading

0 comments on commit be6bd62

Please sign in to comment.