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.

Sign up for GitHub

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

Jump to bottom

Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader #11500

Merged
merged 9 commits into from
Feb 27, 2023
Merged

Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader #11500

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ecf0aaa
deprecate 'kind' and 'path' from the layout reader
mscheltienne Feb 22, 2023
71075a5
add entry to changelog
mscheltienne Feb 22, 2023
dbc0386
fix flake8
mscheltienne Feb 22, 2023
ef9e41d
Merge remote-tracking branch 'upstream/main' into layout
mscheltienne Feb 27, 2023
4d64405
better deprecation
mscheltienne Feb 27, 2023
fcc3968
check the extension
mscheltienne Feb 27, 2023
8517a24
fix layout tests
mscheltienne Feb 27, 2023
ae93ab3
fix flake8
mscheltienne Feb 27, 2023
8e9c3f0
Update doc/changes/latest.inc
larsoner Feb 27, 2023
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: 1 addition & 1 deletion doc/changes/latest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,4 @@ Bugs

API changes
~~~~~~~~~~~
- None yet
- Deprecate arguments ``kind`` and ``path`` from `mne.channels.read_layout` in favor of a common argument ``fname`` (:gh:`11500` by `Mathieu Scheltienne`_)
larsoner marked this conversation as resolved.
Show resolved Hide resolved
78 changes: 59 additions & 19 deletions mne/channels/layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,18 +163,30 @@ def _read_lay(fname):
return box, pos, names, ids


def read_layout(kind, path=None, scale=True):
def read_layout(kind=None, path=None, fname=None, *, scale=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. All existing user code currently will have either read_layout(name, ...) or read_layout(kind=name, ...). It would be nice if at least the first one (which might even be the most common!) continued to work without a deprecation warning if possible. So what if we deprecate differently by doing:

    Suggested change
    def read_layout(kind=None, path=None, fname=None, *, scale=True):
    def read_layout(fname=None, path=None, scale=True, *, kind=None):
    if kind is not None:
    warn(...)
    ...
    if path is not None:
    warn(...)
    ...

    That way read_layout('Vectorview-all') works on 1.3 and also in this PR without any warning.

  2. Since the current default for path is None, we need to use something other than None as the default now to know if a user has actually passed the param or not. otherwise code like this:

    read_layout('VectorView-all', path=None)

    works on 1.3, does not warn the user for passing path=None in 1.4, yet will raise an error about the kwarg not existing come 1.5. We could for example use path='' (I think it's safe to assume no user has done path='' in their code) and do:

    _validate_type(path, ('path-like', None), 'path')
if path != '':
    warn(...)

    as our user-passed-so-need-to-warn check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, it should be better now, thanks!

"""Read layout from a file.

Parameters
----------
kind : str
kind : str | None
The name of the ``.lout`` file (e.g. ``kind='Vectorview-all'`` for
``'Vectorview-all.lout'``).

.. deprecated:: v1.4
The ``kind`` and ``path`` parameters will be removed in version
1.5. Please use the ``fname`` parameter instead.
path : path-like | None
The path of the folder containing the Layout file. Defaults to the
``mne/channels/data/layouts`` folder inside your mne-python
installation.

.. deprecated:: v1.4
The ``kind`` and ``path`` parameters will be removed in version
1.5. Please use the ``fname`` parameter instead.
fname : path-like | str
Either the path to a ``.lout`` or ``.lay`` file or the name of a
built-in layout. c.f. Notes for a list of the available built-in
layouts.
scale : bool
Apply useful scaling for out the box plotting using ``layout.pos``.
Defaults to True.
Expand All @@ -190,7 +202,7 @@ def read_layout(kind, path=None, scale=True):

Notes
-----
Valid ``kind`` arguments are:
Valid ``fname`` (or ``kind``) arguments are:

.. table::
:widths: auto
Expand Down Expand Up @@ -241,22 +253,50 @@ def read_layout(kind, path=None, scale=True):
"""
readers = {".lout": _read_lout, ".lay": _read_lay}

if path is None:
path = Path(__file__).parent / "data" / "layouts"
# kind should be the name as a string, but let's consider the case where
# the path to the file is provided instead.
kind = Path(kind)
if len(kind.suffix) == 0 and (path / kind.with_suffix(".lout")).exists():
kind = kind.with_suffix(".lout")
elif len(kind.suffix) == 0 and (path / kind.with_suffix(".lay")).exists():
kind = kind.with_suffix(".lay")

fname = kind if kind.exists() else path / kind.name
if fname.suffix not in (".lout", ".lay"):
raise ValueError(
"Unknown layout type. Should be of type .lout or .lay."
if fname is None: # deprecated in 1.4
warn(
"Argument 'kind' and 'path' are deprecated in favor of 'fname'.",
DeprecationWarning,
)
kind = fname.stem
if path is None:
path = Path(__file__).parent / "data" / "layouts"
# kind should be the name as a string, but let's consider the case
# where the path to the file is provided instead.
kind = Path(kind)
if (
len(kind.suffix) == 0
and (path / kind.with_suffix(".lout")).exists()
):
kind = kind.with_suffix(".lout")
elif (
len(kind.suffix) == 0
and (path / kind.with_suffix(".lay")).exists()
):
kind = kind.with_suffix(".lay")

fname = kind if kind.exists() else path / kind.name
if fname.suffix not in (".lout", ".lay"):
raise ValueError(
"Unknown layout type. Should be of type .lout or .lay."
)
kind = fname.stem
else:
if kind is not None: # to be removed along the deprecated argument
raise ValueError(
"Argument 'kind' and 'path' should be None if 'fname' is used."
)
if isinstance(fname, str):
# is it a built-in layout?
directory = Path(__file__).parent / "data" / "layouts"
if (directory / fname).exists():
fname = directory / fname
elif (directory / fname).with_suffix(".lout").exists():
fname = (directory / fname).with_suffix(".lout")
elif (directory / fname).with_suffix(".lay").exists():
fname = (directory / fname).with_suffix(".lay")
# if not, it must be a valid path provided as str or Path
fname = _check_fname(fname, "read", must_exist=True, name="layout")
kind = fname.stem
box, pos, names, ids = readers[fname.suffix](fname)

if scale:
Expand Down Expand Up @@ -495,7 +535,7 @@ def find_layout(info, ch_type=None, exclude='bads'):
return generate_2d_layout(xy, ch_names=ch_names, name='custom',
normalize=True)

layout = read_layout(layout_name)
layout = read_layout(fname=layout_name)
if not is_old_vv:
layout.names = _clean_names(layout.names, remove_whitespace=True)
if has_CTF_grad:
Expand Down
50 changes: 42 additions & 8 deletions mne/channels/tests/test_layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,22 +54,44 @@ def _get_test_info():

def test_io_layout_lout(tmp_path):
"""Test IO with .lout files."""
layout = read_layout('Vectorview-all', scale=False)
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
layout = read_layout('Vectorview-all', scale=False)
layout.save(tmp_path / "foobar.lout")
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
layout_read = read_layout(
tmp_path / "foobar.lout", path="./", scale=False
)
assert_array_almost_equal(layout.pos, layout_read.pos, decimal=2)
assert layout.names == layout_read.names
assert "<Layout |" in layout.__repr__()

# with fname
layout = read_layout(fname="Vectorview-all", scale=False)
layout.save(tmp_path / "foobar.lout", overwrite=True)
layout_read = read_layout(
tmp_path / "foobar.lout", path="./", scale=False
fname=tmp_path / "foobar.lout", scale=False,
)
assert_array_almost_equal(layout.pos, layout_read.pos, decimal=2)
assert layout.names == layout_read.names
assert "<Layout |" in layout.__repr__()

# deprecation
with pytest.raises(ValueError, match="'kind' and 'path' should be None"):
layout_read = read_layout(
kind="Vectorview-all", fname=tmp_path / "foobar.lout", scale=False,
)


def test_io_layout_lay(tmp_path):
"""Test IO with .lay files."""
layout = read_layout('CTF151', scale=False)
layout = read_layout(fname="CTF151", scale=False)
layout.save(str(tmp_path / "foobar.lay"))
layout_read = read_layout(
str(tmp_path / "foobar.lay"), path="./", scale=False
fname=tmp_path / "foobar.lay", scale=False
)
assert_array_almost_equal(layout.pos, layout_read.pos, decimal=2)
assert layout.names == layout_read.names
Expand Down Expand Up @@ -143,14 +165,20 @@ def test_make_eeg_layout(tmp_path):
"""Test creation of EEG layout."""
tmp_name = 'foo'
lout_name = 'test_raw'
lout_orig = read_layout(kind=lout_name, path=lout_path)
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
lout_orig = read_layout(kind=lout_name, path=lout_path)
info = read_info(fif_fname)
info['bads'].append(info['ch_names'][360])
layout = make_eeg_layout(info, exclude=[])
assert_array_equal(len(layout.names), len([ch for ch in info['ch_names']
if ch.startswith('EE')]))
layout.save(str(tmp_path / (tmp_name + ".lout")))
lout_new = read_layout(kind=tmp_name, path=tmp_path, scale=False)
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
lout_new = read_layout(kind=tmp_name, path=tmp_path, scale=False)
assert_array_equal(lout_new.kind, tmp_name)
assert_allclose(layout.pos, lout_new.pos, atol=0.1)
assert_array_equal(lout_orig.names, lout_new.names)
Expand All @@ -168,10 +196,16 @@ def test_make_grid_layout(tmp_path):
"""Test creation of grid layout."""
tmp_name = 'bar'
lout_name = 'test_ica'
lout_orig = read_layout(kind=lout_name, path=lout_path)
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
lout_orig = read_layout(kind=lout_name, path=lout_path)
layout = make_grid_layout(_get_test_info())
layout.save(str(tmp_path / (tmp_name + ".lout")))
lout_new = read_layout(kind=tmp_name, path=tmp_path)
with pytest.warns(
DeprecationWarning, match="'kind' and 'path' are deprecated"
):
lout_new = read_layout(kind=tmp_name, path=tmp_path)
assert_array_equal(lout_new.kind, tmp_name)
assert_array_equal(lout_orig.pos, lout_new.pos)
assert_array_equal(lout_orig.names, lout_new.names)
Expand Down