-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add axes argument for montage plotting
#11404
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
Conversation
Allows integrating montage plots into other plots.
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
| @verbose | ||
| def plot_montage(montage, scale_factor=20, show_names=True, kind='topomap', | ||
| show=True, sphere=None, verbose=None): | ||
| show=True, sphere=None, verbose=None, axes=None): |
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.
| show=True, sphere=None, verbose=None, axes=None): | |
| show=True, sphere=None, axes=None, *, verbose=None): |
verbose must always come last. You'll need to update the order of docstring parameter entries too.
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 was worried about backward compatibility with this. Making it a kwarg-only attribute sounds great. So I'll also implement that change of making verbose a kwarg in these functions?
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.
yes. We're sort of in transition to making wider use of kwarg-only parameters. verbose is one that should always be kwarg-only; others are sort of case-by-case and for now are mostly being done in functions/methods with really long parameter lists.
| @copy_function_doc_to_method_doc(plot_montage) | ||
| def plot(self, scale_factor=20, show_names=True, kind='topomap', show=True, | ||
| sphere=None, verbose=None): | ||
| sphere=None, verbose=None, axes=None): |
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.
| sphere=None, verbose=None, axes=None): | |
| sphere=None, axes=None. *, verbose=None): |
ditto
| axes : instance of Axes | instance of Axes3D | None | ||
| Axes to draw the sensors to. If ``kind='3d'``, axes must be an instance | ||
| of Axes3D. If None (default), a new axes will be created. | ||
|
|
||
| .. versionadded:: 1.4.0 |
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.
Ideally this would become
| axes : instance of Axes | instance of Axes3D | None | |
| Axes to draw the sensors to. If ``kind='3d'``, axes must be an instance | |
| of Axes3D. If None (default), a new axes will be created. | |
| .. versionadded:: 1.4.0 | |
| %(axes_plot_sensors)s | |
| .. versionadded:: 1.4.0 |
and then a similar change here:
Lines 939 to 943 in ea88c55
| axes : instance of Axes | instance of Axes3D | None | |
| Axes to draw the sensors to. If ``kind='3d'``, axes must be an instance | |
| of Axes3D. If None (default), a new axes will be created. | |
| .. versionadded:: 0.13.0 |
... and then the duplicated docstring gets added to the docdict in between these two entries:
Lines 283 to 285 in ea88c55
| docdict['axes_plot_projs_topomap'] = _axes_list.format( | |
| 'axes', 'match the number of projectors') | |
| docdict['axes_plot_topomap'] = _axes_base.format('axes', '', '', '') |
If that sounds doable, please give it a try; if not LMK and I can push a commit that does this.
Allows integrating montage plots into other plots.