Deprecate meg=True in pick_types #7823

cbrnr · 2020-05-25T14:29:44Z

Fixes #7710 (change the default meg=True to meg=False in a deprecation cycle.

Fix/prepare examples/tutorials/etc.
Change also in pick_types_forward?
~~Any additional MEG bias we should remove in this PR?~~
~~Make arguments kw-only?~~

mne/io/pick.py

cbrnr · 2020-05-25T17:36:35Z

@larsoner can you check if the logic when to warn is OK like this?

cbrnr · 2020-05-25T20:11:45Z

Now do I catch all deprecation warnings individually or do I just disable this specific DeprecationWarning globally (do we have something like a pytest.ini config file?)?

larsoner · 2020-05-25T20:32:03Z

You will probably need to update all tests that currently fail to now explicitly use meg=True to ensure they work the same way now and once we switch to meg=False as the default

mne/io/pick.py

larsoner · 2020-05-25T20:36:07Z

Also you should push a commit with [circle full] in the message, it will show how many examples also break (we do not allow examples to give dep warnings) and thus also need to be updated

larsoner · 2020-05-26T13:40:04Z

Sphinx-gallery successfully executed 97 out of 190 files subselected by:

Looks like that's 93 examples to change :(

cbrnr · 2020-05-26T14:05:05Z

Looks like that's 93 examples to change :(

Yep, I'm on it.

larsoner · 2020-05-27T00:30:42Z

Change also in pick_types_forward?

Yes I think we should. I imagine this is used much more often internally than it is by users, so this won't inflict nearly as much pain for people than the primary pick_types deprecation will.

Any additional MEG bias we should remove in this PR?

pick_types and related functions are the only MEG-biased ones I can think of offhand

Make arguments kw-only?

pick_types has been around for 10 years now and I don't think we've ever tried to reorder the arguments, so I don't see too much point in doing so. There are probably people who have done pick_types(info, False, True) or `evoked.pick_types(False, True) knowing that meg/eeg are the first two non-info arguments, and I don't see a reason to break their code.

mne/io/pick.py

cbrnr · 2020-05-27T13:23:29Z

Except for my question about _pick_data_or_ica this is ready!

cbrnr · 2020-05-27T16:14:36Z

Last question solved, ready from my side!

larsoner · 2020-05-27T16:15:13Z

needs rebase/merge, conflict

larsoner · 2020-05-27T16:16:14Z

doc/changes/latest.inc


 - The function ``mne.channels.read_dig_captrack`` will be deprecated in version 0.22 in favor of :func:`mne.channels.read_dig_captrak` to correct the spelling error: "captraCK" -> "captraK", by `Stefan Appelhoff`_
+
+- The default argument ``meg=True`` in func:`mne.io.pick_types` will change to ``meg=False`` in version 0.22 by `Clemens Brunner`_


This is documented at the root level so this will be an error

https://mne.tools/stable/generated/mne.pick_types.html

Suggested change

- The default argument ``meg=True`` in func:`mne.io.pick_types` will change to ``meg=False`` in version 0.22 by `Clemens Brunner`_

- The default argument ``meg=True`` in func:`mne.pick_types` and related methods like :meth:`mne.io.Raw.pick_types` and :meth:`mne.Evoked.pick_types` will change to ``meg=False`` in version 0.22 by `Clemens Brunner`_

Just so I understand, where will this be an error (if the doc contains func:mne.io.pick_types)?

where will this be an error

CircleCI will say something about not being able to link to the ref properly, and the build will fail

(alternatively, locally you can do make html_dev-noplot in the doc/ directory then echo $? afterward it should not be zero, and if you look up you should see a warning about not being able to link properly)

... though I seem to remember your previous build being green, so maybe it figures out to go up to mne.pick_types when the mne.io.pick_types link is requested somehow?

The previous CircleCI build was all green with exit code 0, yes.

Looks like it was indeed green but shouldn't have been because the link did not get created correctly, maybe some latest Sphinx change made this fail to link without raising an error, not sure...

the linking failure is due to lack of a colon at the beginning of func

Grrr... thanks @drammock . I'll see if changing our default_role to something other than "autolink" linke "py:obj" would have made this error show up, in which case we should change it

I've inserted the missing colon and now use mne.pick_types.

…in pick_types)

hoechenberger · 2020-05-27T16:57:04Z

mne/channels/channels.py

+            If True include MEG channels. If string it can be 'mag', 'grad',
+            'planar1' or 'planar2' to select only magnetometers, all
+            gradiometers, or a specific type of gradiometer.


Suggested change

If True include MEG channels. If string it can be 'mag', 'grad',

'planar1' or 'planar2' to select only magnetometers, all

gradiometers, or a specific type of gradiometer.

If ``True``, include MEG channels. If string, it can be ``'mag'``,

``'grad``', ``'planar1'`` or ``'planar2'`` to select only magnetometers

all gradiometers, or a specific type of gradiometer.

hoechenberger

Just added a few suggestions

hoechenberger · 2020-05-27T16:58:30Z

mne/channels/channels.py

+            If True include CTF / 4D reference channels. If 'auto', reference
+            channels are included if compensations are present and ``meg`` is
+            not False. Can also be the string options for the ``meg``
+            parameter.


Suggested change

If True include CTF / 4D reference channels. If 'auto', reference

channels are included if compensations are present and ``meg`` is

not False. Can also be the string options for the ``meg``

parameter.

If ``True``, include CTF / 4D reference channels. If ``'auto'``, reference

channels are included if compensations are present and ``meg`` is

not ``False``. Can also be the string options for the ``meg``

parameter.

hoechenberger · 2020-05-27T16:59:34Z

mne/io/pick.py

+        If True include MEG channels. If string it can be 'mag', 'grad',
+        'planar1' or 'planar2' to select only magnetometers, all gradiometers,
+        or a specific type of gradiometer.


Suggested change

If True include MEG channels. If string it can be 'mag', 'grad',

'planar1' or 'planar2' to select only magnetometers, all gradiometers,

or a specific type of gradiometer.

If ``True``, include MEG channels. If string, it can be ``'mag'``

``'grad'``, ``'planar1'`` or ``'planar2'`` to select only

magnetometers, all gradiometers, or a specific type of gradiometer.

hoechenberger · 2020-05-27T17:00:41Z

mne/io/pick.py

+        If True include CTF / 4D reference channels. If 'auto', reference
+        channels are included if compensations are present and ``meg`` is not
+        False. Can also be the string options for the ``meg`` parameter.


Suggested change

If True include CTF / 4D reference channels. If 'auto', reference

channels are included if compensations are present and ``meg`` is not

False. Can also be the string options for the ``meg`` parameter.

If ``True``, include CTF / 4D reference channels. If ``'auto'``, reference

channels are included if compensations are present and ``meg`` is not

``False``. Can also be the string options for the ``meg`` parameter.

hoechenberger · 2020-05-27T17:01:55Z

mne/io/pick.py

+        If True include MEG channels. If string it can be 'mag', 'grad',
+        'planar1' or 'planar2' to select only magnetometers, all gradiometers,
+        or a specific type of gradiometer.


Suggested change

If True include MEG channels. If string it can be 'mag', 'grad',

'planar1' or 'planar2' to select only magnetometers, all gradiometers,

or a specific type of gradiometer.

If ``True``, include MEG channels. If string, it can be ``'mag'``,

``'grad'``, ``'planar1'`` or ``'planar2'`` to select only

magnetometers, all gradiometers, or a specific type of gradiometer.

cbrnr · 2020-05-27T17:45:51Z

@hoechenberger do we really always have to double-backtick literals in docstrings? I know that this makes it show up as <code>, but IIRC we've had this discussion in the past and decided that it's not worth the effort (and additional characters).

drammock · 2020-05-27T17:52:30Z

IIRC we've had this discussion in the past and decided that it's not worth the effort (and additional characters)

I don't understand the reasoning. We're not charged per-character, and the effort is about as minimal as it gets (press the key your finger is already on, twice instead of once). I'll concede that the changelog is probably the least important place to get this right (much more important in tutorials and docstrings), but remembering / enforcing to do it correctly here is a good way to reinforce the behavior so that it also happens everywhere else.

hoechenberger · 2020-05-27T17:55:58Z

+100 @drammock

cbrnr · 2020-05-27T18:19:06Z

It's harder to read in plain text when I see everything surrounded by double backticks. But whatever, if we now do that I will change it.

cbrnr · 2020-05-27T20:22:09Z

meg : bool | str
    If ``True` include MEG channels. If string it can be ``'mag'``,
    ``'grad'``, ``'planar1'`` or ``'planar2'`` to select only
    magnetometers, all gradiometers, or a specific type of gradiometer.

You don't think this is harder to read (in plain text) (especially the literal strings)? Also, why change this only in a few selected parameter descriptions and not all? I don't want to do it here, if you think this is important please do it in a new PR.

larsoner · 2020-05-27T20:24:48Z

You don't think this is harder to read (in plain text) (especially the literal strings)?

I find it a little bit harder, but not too much. And to me the RST rendering gain is worth the plain-text rendering loss. I'm fine with someone else working on this in another PR, though

larsoner

LGTM, the changeset is actually smaller than I expected!

hoechenberger · 2020-05-27T21:11:23Z

You don't think this is harder to read (in plain text) (especially the literal strings)?

I think it's not easy to read without syntax highlighting that helps with this.

That being said, our users view the rendered output, and many IDEs provide rendered docstrings as well. So unless you're actually working on the docstrings, this shouldn't really affect you, no?

Also, why change this only in a few selected parameter descriptions and not all?

Because we're already inconsistent anyway and changing some bits "as we go" in PRs that change some lines of docstrings brings us a little closer to overall more consistently rendered docs.

I don't want to do it here

There's no need to, it was merely suggestions, no blockers from my end. Sorry if it came across otherwise!

hoechenberger · 2020-05-28T06:27:01Z

FWIW, I'm using VS Code with the Highlight extension and the following setting:

    "highlight.regexes": {
        "(``)(.+?)(``)": [
            {"color": "#d88e73"},
            {"color": "#3c9ddb"},
            {"color": "#d88e73"}
        ]
    }

This produces:

Maybe some of you may find this useful.

cbrnr · 2020-05-28T06:55:13Z

That's pretty neat! Is there anything similar for PyCharm?

hoechenberger · 2020-05-28T07:19:43Z

That's pretty neat! Is there anything similar for PyCharm?

I'm afraid not, at least I haven't found anything so far :( There is a related support request from ~2 years ago:
https://intellij-support.jetbrains.com/hc/en-us/community/posts/360000082599-Custom-syntax-highlighting

cbrnr · 2020-05-28T07:28:57Z

OK, that's a pity. Though it wouldn't really solve the issue, because users (as opposed to devs) will read docstrings mostly in their REPL (e.g. IPython), which means they will see just plain text without any fancy rST highlighting. I think enclosing every literal in double backticks is just overkill.

Also, it violates the numpydoc style recommendation that variables should be enclosed in single backticks: https://numpydoc.readthedocs.io/en/latest/format.html.

hoechenberger · 2020-05-28T07:48:58Z

Thanks @cbrnr
@drammock, would there be a way to change the way we mark variable names and constants to make things more readable in plain text? Maybe we should open a new issue about this…

drammock · 2020-05-28T16:19:40Z

users (as opposed to devs) will read docstrings mostly in their REPL (e.g. IPython)

@cbrnr Do we actually have evidence for this one way or the other? Anyone who uses Spyder will typically see HTML-rendered docs, for example.

would there be a way to change the way we mark variable names and constants to make things more readable in plain text?

@hoechenberger our sphinx config was recently changed so that anything surrounded by single ticks (which previously rendered as italics, redundant with single asterisks for yielding italics) will now always be treated as an attempted link or cross-reference by the build system. This allows Sphinx to catch errors like the recently seen

func:`some_function_name`

where func: was missing a leading colon. Previously it rendered as func:italicized_function_name and raised no error / warning. This change also means that the role prefix (:func:, :class: etc) can be omitted in most cases, which will allow us to simplify the plain-text docstrings considerably. It's a trade-off.

cbrnr · 2020-05-29T08:08:35Z

@cbrnr Do we actually have evidence for this one way or the other? Anyone who uses Spyder will typically see HTML-rendered docs, for example.

I only have anecdotal evidence from coworkers and students in my Python course, where the majority seems to be using plain-text docstrings (mostly via the IPython shortcut ?command or command?). Plain-text docs are always available no matter what editor or IDE you're using (even if HTML rendering is supported). Many editors don't support HTML rendering of docstrings out of the box (e.g. VS Code, PyCharm) - or at least I haven't figured out how to view HTML-rendered docs. It seems like Spyder is the only editor where this works out of the box.

👍 for the simplification! Regarding the use of double backticks, do we have docstring guidelines somewhere? If not I think it would be a good addition to have such a document in the contributing section.

cbrnr · 2020-05-29T08:11:11Z

@drammock @hoechenberger do you have any additional comments (let's do docstring formatting changes in another PR though)?

hoechenberger · 2020-05-29T08:20:47Z

@cbrnr LGTM!

hoechenberger · 2020-05-29T08:21:17Z

@drammock I think you can merge this if you're happy

drammock · 2020-05-29T14:07:30Z

do we have docstring guidelines somewhere? If not I think it would be a good addition to have such a document in the contributing section.

added to #6266

drammock · 2020-05-29T14:08:08Z

thanks @cbrnr !

* upstream/master: DOC: Order added reference FIX: Working working version ENH: More efficient actually working csd, needs review MAINT: Update dataset and add constant test (mne-tools#7866) DOC: update link to glasser supplementary info; convert to footbib (mne-tools#7864) FIX: Fix subtract_evoked with decim (mne-tools#7855) FIX: Fix reading of old TFRs (mne-tools#7851) DOC: Add evoked movecomp to example (mne-tools#7852) Deprecate meg=True in pick_types (mne-tools#7823) ENH: Allow reading broken file (mne-tools#7846) MRG, ENH: Add mixed source estimate support to compute_source_morph (mne-tools#7734) FIX: Fix bug with metadata and event_repeated (mne-tools#7733) MRG, ENH: Add axes to plot_evoked_white (mne-tools#7831) MRG, ENH: Add example of projection to source space (mne-tools#7705)

cbrnr marked this pull request as draft May 25, 2020 14:30

larsoner reviewed May 25, 2020

View reviewed changes

mne/io/pick.py Outdated Show resolved Hide resolved

larsoner reviewed May 25, 2020

View reviewed changes

mne/io/pick.py Show resolved Hide resolved

larsoner mentioned this pull request May 27, 2020

MRG, MAINT: Show how picks work for planars #7833

Merged

cbrnr commented May 27, 2020

View reviewed changes

mne/io/pick.py Show resolved Hide resolved

cbrnr force-pushed the pick-types branch from aab05ac to 64c2fb7 Compare May 27, 2020 08:42

cbrnr marked this pull request as ready for review May 27, 2020 13:22

cbrnr added 6 commits May 27, 2020 18:15

Deprecate meg=True in pick_types

2b770b8

More informative warning

439d50a

Only warn if there are MEG channels in the data and meg=default arg

bc40192

Fix unbound variable

88160ce

[circle full] Test deprecation warning

e67d6c9

Add docstring

6f6f015

larsoner reviewed May 27, 2020

View reviewed changes

cbrnr added 6 commits May 27, 2020 18:16

Add changelog entry

5567245

[circle full] Fix first batch

ed711c2

More fixes

ad93e56

Fix two more tests

37c62de

Change meg=None in pick_types_forward (deprecation warning triggered …

a7ffe74

…in pick_types)

[circle full] Minor formatting change to trigger full docs build

48aec3f

cbrnr force-pushed the pick-types branch from f31cfc9 to 48aec3f Compare May 27, 2020 16:17

cbrnr mentioned this pull request May 27, 2020

Possible bug in _pick_data_or_ica #7836

Closed

hoechenberger reviewed May 27, 2020

View reviewed changes

Insert missing colon, use mne.pick_types

0050295

larsoner approved these changes May 27, 2020

View reviewed changes

hoechenberger approved these changes May 27, 2020

View reviewed changes

drammock merged commit 002a5cf into mne-tools:master May 29, 2020

cbrnr deleted the pick-types branch May 29, 2020 14:25


		- The function ``mne.channels.read_dig_captrack`` will be deprecated in version 0.22 in favor of :func:`mne.channels.read_dig_captrak` to correct the spelling error: "captraCK" -> "captraK", by `Stefan Appelhoff`_

		- The default argument ``meg=True`` in func:`mne.io.pick_types` will change to ``meg=False`` in version 0.22 by `Clemens Brunner`_

Uh oh!

Deprecate meg=True in pick_types #7823

Deprecate meg=True in pick_types #7823

Uh oh!

Conversation

cbrnr commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cbrnr commented May 25, 2020

Uh oh!

cbrnr commented May 25, 2020

Uh oh!

larsoner commented May 25, 2020

Uh oh!

Uh oh!

larsoner commented May 25, 2020

Uh oh!

larsoner commented May 26, 2020

Uh oh!

cbrnr commented May 26, 2020

Uh oh!

larsoner commented May 27, 2020

Uh oh!

Uh oh!

cbrnr commented May 27, 2020

Uh oh!

cbrnr commented May 27, 2020

Uh oh!

larsoner commented May 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbrnr commented May 27, 2020

Uh oh!

drammock commented May 27, 2020

Uh oh!

hoechenberger commented May 27, 2020

Uh oh!

cbrnr commented May 27, 2020

Uh oh!

cbrnr commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented May 27, 2020

cbrnr commented May 25, 2020 •

edited

Loading

hoechenberger May 27, 2020 •

edited

Loading

hoechenberger May 27, 2020 •

edited

Loading

cbrnr commented May 27, 2020 •

edited

Loading

hoechenberger commented May 27, 2020 •

edited

Loading

hoechenberger commented May 28, 2020 •

edited

Loading