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 'kind' and 'path' in favor of 'fname' in the layout reader #11500

Merged
merged 9 commits into from
Feb 27, 2023

Conversation

mscheltienne
Copy link
Member

Closes #11483

As discussed in #11483, those 2 arguments don't play well and could be replaced by a common explicit fname argument.

@@ -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!

@mscheltienne mscheltienne marked this pull request as draft February 22, 2023 17:33
@mscheltienne
Copy link
Member Author

I don't think the CI failure is related to the PR.

@mscheltienne mscheltienne marked this pull request as ready for review February 27, 2023 14:39
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM, will merge once CIs come back happy again (other than the notebook one, which I'll investigate separately)!

doc/changes/latest.inc Outdated Show resolved Hide resolved
@larsoner larsoner merged commit 0efe50b into mne-tools:main Feb 27, 2023
@mscheltienne mscheltienne deleted the layout branch February 27, 2023 17:35
larsoner added a commit to larsoner/mne-python that referenced this pull request Mar 1, 2023
* upstream/main: (264 commits)
  BUG: Fix deprecated API usage in example (mne-tools#11512)
  Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500)
  EGI/MFF events outside EEG recording should not break the code (mne-tools#11505)
  fixed annotations error on export (mne-tools#11435)
  DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506)
  BUG: updates for MPL 3.7 compatibility (mne-tools#11409)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arguments of the layout reader
2 participants