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

deprecate empty list as indicator for marking all channels #1307

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/whats_new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ Detailed list of changes
- :func:`mne_bids.read_raw_bids` no longer warns about unit changes in channels upon reading, as that information is taken from ``channels.tsv`` and judged authorative, by `Stefan Appelhoff`_ (:gh:`1282`)
- MEG OPM channels are now experimentally included, by `Amaia Benitez`_ (:gh:`1222`)
- :func:`mne_bids.mark_channels` will no longer create a ``status_description`` column filled with ``n/a`` in the ``channels.tsv`` file, by `Stefan Appelhoff`_ (:gh:`1293`)
- :func:`mark_channels(..., ch_names=[]) <mne_bids.mark_channels>` now raises a deprecation warning, and in future its behavior will change from marking *all* channels to marking *no* channels; to avoid the warning use ``mark_channels(..., ch_names="all")``, by `Daniel McCloy`_ (:gh:`1307`)


🛠 Requirements
^^^^^^^^^^^^^^^
Expand Down
5 changes: 3 additions & 2 deletions mne_bids/tests/test_write.py
Original file line number Diff line number Diff line change
Expand Up @@ -2490,7 +2490,7 @@ def test_mark_channels(
# Mark `existing_ch_names` as bad in raw and sidecar TSV before we
# begin our actual tests, which should then add additional channels
# to the list of bads, retaining the ones we're specifying here.
mark_channels(ch_names=[], bids_path=bids_path, status="good", verbose=False)
mark_channels(ch_names="all", bids_path=bids_path, status="good", verbose=False)
_bids_validate(bids_root)
raw = read_raw_bids(bids_path=bids_path, verbose=False)
# Order is not preserved
Expand Down Expand Up @@ -2560,7 +2560,8 @@ def test_mark_channel_roundtrip(tmp_path):

ch_names = raw.ch_names
# first mark all channels as good
mark_channels(bids_path, ch_names=[], status="good", verbose=False)
with pytest.warns(FutureWarning, match=r"`mark_channels\(\.\.\., ch_names=\[\]\)`"):
mark_channels(bids_path, ch_names=[], status="good", verbose=False)
tsv_data = _from_tsv(channels_fname)
assert all(status == "good" for status in tsv_data["status"])

Expand Down
29 changes: 23 additions & 6 deletions mne_bids/write.py
Original file line number Diff line number Diff line change
Expand Up @@ -2522,8 +2522,14 @@ def mark_channels(bids_path, *, ch_names, status, descriptions=None, verbose=Non
type (e.g., only EEG or MEG data) is present in the dataset, it will be
selected automatically.
ch_names : str | list of str
The names of the channel(s) to mark with a ``status`` and optionally a
``description``. Can be an empty list to indicate all channel names.
The name(s) of the channel(s) to mark with a ``status`` (and optionally a
``description``). The special value ``"all"`` will mark all channels.

.. versionchanged:: 0.16
The behavior of passing an empty list will change in version 0.17. In version
0.16 and older, an empty list would mark *all* channels. In version 0.17 and
newer, an empty list will be a no-op (no channels will be marked/changed).

status : 'good' | 'bad' | list of str
The status of the channels ('good', or 'bad'). If it is a list, then must be a
list of 'good', or 'bad' that has the same length as ``ch_names``.
Expand Down Expand Up @@ -2586,10 +2592,21 @@ def mark_channels(bids_path, *, ch_names, status, descriptions=None, verbose=Non

# if an empty list is passed in, then these are the entire list
# of channels
if ch_names == []:
ch_names = tsv_data["name"]
elif isinstance(ch_names, str):
ch_names = [ch_names]
if list(ch_names) == []: # casting to list avoids error if ch_names is np.ndarray
warn(
"In version 0.17, the behavior of `mark_channels(..., ch_names=[])` will "
"change, from marking *all* channels to marking *no* channels. Pass "
"ch_names='all' instead of ch_names=[] to keep the old behavior and "
"avoid this warning.",
FutureWarning,
)
ch_names = "all"
# TODO ↑↑↑ remove prior conditional block after 0.16 release ↑↑↑
if isinstance(ch_names, str):
if ch_names == "all":
ch_names = tsv_data["name"]
else:
ch_names = [ch_names]

# set descriptions based on how it's passed in
if isinstance(descriptions, str):
Expand Down
Loading