-
Notifications
You must be signed in to change notification settings - Fork 1.4k
FIX, DOC: Bug in plot_evoked_joint API #13159
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
base: main
Are you sure you want to change the base?
Conversation
- plot_evoked_joint had the default `exclude=None` but actually `None` is not a valid argument for this parameter! it breaks! - Also, this default was not consistent with `mne.Evoked.plot_joint` which had the default `exclude="bads"` - Further, the docstring from plot_evoked_joint got copied to mne.Evoked.plot_joint, creating a discrepancy bt the docstring and the code (i.e. doc says default is None, but default was actually "bads")
@@ -1824,9 +1824,10 @@ def plot_evoked_joint( | |||
axes are passed make sure to set ``title=None``, otherwise some of your | |||
axes may be removed during placement of the title axis. | |||
%(picks_all)s | |||
exclude : None | list of str | 'bads' | |||
exclude : list of str | 'bads' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we change this to something like exclude : empty list | list of str | 'bads'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so empty list
is covered by list of str
well enough I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test that exclude=None
raises an error? Should it?
@@ -1824,9 +1824,10 @@ def plot_evoked_joint( | |||
axes are passed make sure to set ``title=None``, otherwise some of your | |||
axes may be removed during placement of the title axis. | |||
%(picks_all)s | |||
exclude : None | list of str | 'bads' | |||
exclude : list of str | 'bads' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so empty list
is covered by list of str
well enough I think
Fixes #13157
FYI It's a little more complicated that I thought...
The function plot_evoked_joint does have the default
exclude=None
, which actually does not work! 🚨 But the docstring was technically inline with theAPIfunction signature:mne-python/mne/viz/evoked.py
Lines 1794 to 1857 in df4a3f1
The method evoked.plot_joint has a default
exclude="bads"
. The docstring is copied from the functionplot_evoked_joint
to the methodevoked.plot_joint
which is where the docstring-to-code discrepancy arises:mne-python/mne/evoked.py
Lines 772 to 792 in 8f83332
TL;DR
We can update the docstring but we have to change
mne-python/mne/viz/evoked.py
Line 1800 in df4a3f1
to
exclude="bads"