From 23503f12a039b02552fa5cebdc8f78af0d9dac83 Mon Sep 17 00:00:00 2001 From: Daniel McCloy Date: Mon, 23 Sep 2024 15:22:03 -0500 Subject: [PATCH] deprecate empty list as indicator for marking all channels (#1307) --- doc/whats_new.rst | 2 ++ examples/mark_bad_channels.py | 4 +-- mne_bids/commands/mne_bids_mark_channels.py | 2 +- mne_bids/inspect.py | 2 +- mne_bids/tests/test_write.py | 5 ++-- mne_bids/write.py | 29 ++++++++++++++++----- 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/doc/whats_new.rst b/doc/whats_new.rst index 9ae150b71..42a42a725 100644 --- a/doc/whats_new.rst +++ b/doc/whats_new.rst @@ -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=[]) ` 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 ^^^^^^^^^^^^^^^ diff --git a/examples/mark_bad_channels.py b/examples/mark_bad_channels.py index ce4e45309..ecc5a7256 100644 --- a/examples/mark_bad_channels.py +++ b/examples/mark_bad_channels.py @@ -164,9 +164,9 @@ # %% # Lastly, if you're looking for a way to mark all channels as good, simply -# pass an empty list as ``ch_names``, combined with ``overwrite=True``: +# pass ``ch_names="all"``, combined with ``overwrite=True``: -bads = [] +bads = "all" mark_channels(bids_path=bids_path, ch_names=bads, status="bad", verbose=False) raw = read_raw_bids(bids_path=bids_path, verbose=False) diff --git a/mne_bids/commands/mne_bids_mark_channels.py b/mne_bids/commands/mne_bids_mark_channels.py index 04787169b..11a3777b9 100644 --- a/mne_bids/commands/mne_bids_mark_channels.py +++ b/mne_bids/commands/mne_bids_mark_channels.py @@ -98,7 +98,7 @@ def run(): parser.error("You must specify some --ch_name parameters.") status = opt.status - ch_names = [] if opt.ch_names == [""] else opt.ch_names + ch_names = "all" if opt.ch_names == [""] else opt.ch_names bids_path = BIDSPath( subject=opt.subject, session=opt.session, diff --git a/mne_bids/inspect.py b/mne_bids/inspect.py index 445538375..4882dd9dc 100644 --- a/mne_bids/inspect.py +++ b/mne_bids/inspect.py @@ -446,7 +446,7 @@ def _save_bads(*, bads, descriptions, bids_path): The values to be written to the `status_description` column. """ # We first make all channels not passed as bad here to be marked as good. - mark_channels(bids_path=bids_path, ch_names=[], status="good") + mark_channels(bids_path=bids_path, ch_names="all", status="good") mark_channels( bids_path=bids_path, ch_names=bads, status="bad", descriptions=descriptions ) diff --git a/mne_bids/tests/test_write.py b/mne_bids/tests/test_write.py index c9c2a18eb..ffbb7de3b 100644 --- a/mne_bids/tests/test_write.py +++ b/mne_bids/tests/test_write.py @@ -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 @@ -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"]) diff --git a/mne_bids/write.py b/mne_bids/write.py index 3399f21a9..b7cf34d02 100644 --- a/mne_bids/write.py +++ b/mne_bids/write.py @@ -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``. @@ -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):