Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: addax
kwarg to every pyplot function#9111
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 ourterms of service andprivacy 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
e95b2b8
to4d4662e
CompareWhat's the benefit as opposed to just using the OO API, if you really want to specify the figure/axis? |
This is the opening pass at something I have been thinking about for a while. The very short version is that there is currently a major difference between 'built in' plotting ( defmy_plotter(*data,ax,**style): ... This is bread-crumbs leading that direction. |
This actually looks like a reasonable argument (as much as I would like not to admit it...) |
Yeah, this is pretty much the same proposal from a few years ago that gotsidelined to focus on getting the data kwarg into the v1.5 release, isn'tit?I totally get the argument, I just don't know how well I like that codingstyle. Feels very basemap-ish. Whatever happened to the idea of plottingobjects? …On Mon, Aug 28, 2017 at 11:37 AM, Antony Lee ***@***.***> wrote: This actually looks like a reasonable argument (as much as I would like not to admit it...) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-N7mVbidz89QregvZ__Vt6Tz1ch1ks5sct6mgaJpZM4PEATz> . |
In general I'm ok with the idea. I do think we should think about the idea of plotting objects in this context before proceeding, though. |
Also what's wrong with just using sca() in that case?
which is probably going to be less repetitive that having to pass ax again and again. |
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.
If for no other reason than that this allows using thepyplot
interface with an explicit axes, I'm 💯 on this.
I think that this and more complex objects are orthogonal. The objects are about making more complex plots easier to update and work with (ex errorbars) and this is about letting third-party code, or code that takes in not-arrays as input feel like first class citizens next to the 'official'. I expect there to be plotting functions that take in very non-array things (like a partial sql query!). It also provides a smoother path to leaving the fully implicit world to the full OO world. |
54215b7
to1bbbed8
CompareThis is actually pass 2 at this, see#4488 |
b08714c
tof26efd9
Comparedef test_explict_ax(): | ||
fig, (ax1, ax2) = plt.subplots(2) | ||
plt.plot(range(5)) | ||
plt.plot(range(5)[::-1], ax=ax1) |
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 would just test that len(ax1.lines) == 2 instead of doing an image comp.
test name has typo ("explicit")
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.
There should be one line per axes (gca()
is surprising! ax2 was created last so it is 'current')
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.
you got me :-)
but still you should be able to just test how many lines there are on each axis.
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.
Done
tools/boilerplate.py Outdated
@@ -316,12 +318,13 @@ def format_value(value): | |||
# A gensym-like facility in case some function takes an | |||
# argument named washold, ax, or ret | |||
washold, ret, ax= 'washold', 'ret', 'ax' | |||
washold, ret= 'washold', 'ret' |
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.
the comment above needs to be updated
side point: I think we should just throw an exception in this case -- the parameter name is part of the API so renaming it seems silly
198eb48
toccabef7
Comparetools/boilerplate.py Outdated
bad = set(args) | {varargs, varkw} | ||
while washold in bad or ret in bad or ax in bad: | ||
if 'ax' is set(args): |
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 suggesting to do the same for washold and ret below.
Even if this turns out to be the way to go, bringing in such a major strategic change with so little discussion and review is not good. Suggestion: leave this out of 2.1, and make it theproposed centerpiece of 2.2, with avery short release cycle--say, Nov. 1 as the target. |
It was worth a shot! I am taking@efiring 's comment as a veto of this going in for 2.1. Delaying to 2.2 in Nov would give us time to write some collateral around this change, flesh out some of the other implications, and retroactively write a MEP which is probably for the best |
Having though a bit about this, right now I belive my preferred take on such an API ("to make pyplot and external functions more symmetrical") would be
which seems better than having to pass the same ax again and again. |
However this means inside of every function there must be a call to Havingno current figure/ax is a non-starter. Having a context manager that resets the current axes to what it was before would be a reasonable thing though.
|
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.
In terms of code, this looks good, but the PR introduces a backward incompatible change.
On the documentation front, a lot of the docstrings need to be updated to reflect the addition of an optional keyword argument. I'm happy to push the missing documentation.
I'm also concerned about the unit tests. While indeed, this has been tested on one of the pyplot function, I think we should maybe write smoke tests coverage all pyplot functions concerned by this change.
lib/matplotlib/pyplot.py Outdated
@@ -928,7 +970,7 @@ def delaxes(*args): | |||
if not len(args): | |||
ax = gca() | |||
else: | |||
ax = args[0] | |||
ax, = args |
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.
This will fail if args has more arguments than one. If args is can only be one optional argument, why not change *args into an actual optional keyword argument? That would make the code more readible, and more easily documentable than currently.
Note that because of this line we are currently introducing a backward compatible change with this PR.
lib/matplotlib/pyplot.py Outdated
@@ -1328,19 +1374,20 @@ def tight_layout(pad=1.08, h_pad=None, w_pad=None, rect=None): | |||
coordinate that the whole subplots area (including | |||
labels) will fit into. Default is (0, 0, 1, 1). | |||
""" | |||
fig = gcf() | |||
if fig is 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.
The docstring needs to be update to reflect the addition of an optional keyword argument.
lib/matplotlib/pyplot.py Outdated
fig.tight_layout(pad=pad, h_pad=h_pad, w_pad=w_pad, rect=rect) | ||
def box(on=None): | ||
def box(on=None, ax=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.
The docstring needs to be update to reflect the addition of an optional keyword argument.
@@ -1376,6 +1423,9 @@ def title(s, *args, **kwargs): | |||
loc : {'center', 'left', 'right'}, str, optional | |||
Which title to set, defaults to 'center' | |||
ax : matplotlib.axes.Axes, optional |
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.
👍
lib/matplotlib/pyplot.py Outdated
@@ -1389,7 +1439,7 @@ def title(s, *args, **kwargs): | |||
properties. | |||
""" | |||
returngca().set_title(s, *args, **kwargs) | |||
return_pop_or_gca(kwargs).set_title(s, *args, **kwargs) |
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.
The docstring needs to be update to reflect the addition of an optional keyword argument.
98c8f53
to2cb8655
Comparerebased to keep the dream alive. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
timhoffm commentedApr 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
As a side-note: The place where the I've marked this as |
This allows passing `ax=some_ax` or `fig=some_fig` into all of thepyplot plotting functions.
I could not quickly figure out how to convince the generated annotations to not use the fully qualified class names. |
Bit confused why it was not hitting these before.
mypy is still also a bit unhappy (it does not like forwarding returns through unconditionally...I guess the fix would be to make the generating code smart enough to drop the return if the wrapped function claims to only ever return |
This allows passing
ax=some_ax
into all of the pyplot plottingfunctions.
PR Checklist