-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: first draft of ensure_ax decorator #4488
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
@matplotlib/developers Thoughts on this? I would like to present some version of this in my scipy talk so would like feed back as soon as possible. |
I haven't tested it yet, but it looks good. |
@efiring Thoughts on docstring changes? |
@tacaswell, the docstring change looks good. |
def register_func(func): | ||
""" | ||
Registers a function to be wrapped and made available in the | ||
pyplot namespace for convenient interactive command line use. |
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.
command-line use? Might be misleading. Would probably be better as just "interactive use".
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 want to keep something in there so we don't encourage people to use these
functions in other functions.
I am a bit hesitant to include this at all for exactly that reason.
On Tue, Jul 7, 2015, 3:06 PM Benjamin Root notifications@github.com wrote:
In lib/matplotlib/pyplot.py
#4488 (comment):
else:
ax = gca()
return func(s, ax, _args, *_kwargs)
- pre_doc = inner.doc
- if pre_doc is None:
pre_doc = ''
- else:
pre_doc = dedent(pre_doc)
- inner.doc = _ENSURE_AX_DOC + pre_doc
- return inner
+def register_func(func):
- """
- Registers a function to be wrapped and made available in the
- pyplot namespace for convenient interactive command line use.
command-line use? Might be misleading. Would probably be better as just
"interactive use".—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4488/files#r34083841.
@tacaswell Does this need an example? At least I feel a bit confused by it as I don't see it used anywhere (and I haven't learnt about decorators yet)... |
@wraps(func) | ||
def inner(*args, **kwargs): | ||
if 'ax' in kwargs: | ||
ax = kwargs.pop('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.
Get rid of the None
. If ax
is (somehow?) set to None
here the user function that expects an axes will explode
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.
Well it looks meaningless anyhow as we have checked that we have 'ax' as a keyword argument, thus it will never use the default...
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.
Good catch on that, I think I changed how this logic was going to be handled half way through writing it.
Looks good. I doubt this is the case, but just to anticipate a question from the peanut gallery: does this add significant overhead vs. the current approach? |
One other question, what does ENH stand for? (Also I have seen MNT and the likes in PR titles). |
ENH -- Enhancement On Wed, Jul 8, 2015 at 11:33 AM, OceanWolf notifications@github.com wrote:
|
I'm assuming it's —Reply to this email directly or view it on GitHub. |
This is shameless stolen from numpy, there is a key in their development On Wed, Jul 8, 2015, 11:24 AM Jouni K. Seppänen notifications@github.com
|
that key is a little below this link. |
|
||
func(*args, **kwargs) | ||
func(ax, *args, **kwargs) | ||
func(.., ax=ax) |
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.
Maybe replace each of these "func" strings with "{0}" so the actual name can be interpolated. See below.
@njsmith expressed grave concern about the registration function, please don't merge this until that gets sorted (I think I am leaning towards agreeing with him that this is a bad idea). |
@ericdill Added one of the decorators you wanted 🍺 |
I think this is ready to merge. The addition of |
pre_doc = '' | ||
else: | ||
pre_doc = dedent(pre_doc) | ||
inner.__doc__ = _ENSURE_AX_DOC.format(func=func.__name__, obj='') + pre_doc |
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.
Shouldn't the new information be at the end? This way it even overwrites the first line?
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 put it at the top because it is doing semi-scary things to the signature (moving a required arg to the end). Is there a standard way of doing this when using partials?
Internally we do not do a good job of having a meaningful first line.
In phone conversation with @efiring he suggesting changing 'quick_axes' to `gna` for 'get new axes' in analogy and contrast to `gca` -> 'get current axes (and create one if you have to)'.
a0bb42b
to
e398ad8
Compare
rebased to fix import conflict in pyplot |
👍 - you've much more time thinking about improvements to a very old codebase than anybody else, and you've got some great ideas - I think for the sake of the community it is well worth writing small MEPs which we can all buy-in to. For that reason, I don't believe this is a release critical PR, and I wouldn't want to see a release held up for it. Maybe we need a "a core dev is advocating this for the next release, and we should do our best to make sure it gets in" milestone 😄 |
I see a lot of these "Release critical" things as extra features that might help a few people... I don't understand what makes something "Release critical". In debian, they have a strict set of goals (MEPs) that they want to achieve for the next release, like ensuring everything supports utf8... 👍 to needing more MEPs, but I don't mind them large, or at least overarching... we can always split up MEPs into smaller sub MEPs and those into even smaller PRs... for instance we have a large backend refactor now and that got divided into two sub-MEPs, MEP22 and MEP27, and those get broken down into smaller PRs. One thing we do need before that though, a way of sorting out the MEPs, I like that we now submit MEPs through PRs, but... we now have a problem with numbering, if we have multiple MEPs sitting in PRs not yet merged, it becomes a nightmare to keep track of numbers, and we will have merge conflicts, etcetera, so we need to sort that out... |
Okay, I have just started writing an MEP on an axes refactor/overhaul, written from the ground up. Depending on whether we achieve full Backwards Compatibility with the MEP looks by the end I guess will determine whether this will go out in 2.x or 3.0. But first to figure out how to do MEPs based on my points above... |
I expected this to be a relatively non-controversial PR. @OceanWolf With the exception of the labeled data unpacking (which is a release critical feature, we need to address the reality of labeled data being wide-spread) all of these feature have been in the milestone for several months. To be fair, they are there because I put them there, but no one argued at the time and as I am running the release I am going to claim that as my prerogative 😉. The main thing that makes these 'critical' is that they are new features (which can't be merged once we have an RC), not bug fixes (which can be). @pelson I am not sure what your hesitation about the decorators. I am happy to mark these as experimental if that makes you more comfortable. The logic on how I got to them was basically
The weakest point that I see in there is my dislike of breaking Long term I think the list of figure/axes creation methods in pyplot should go down to:
and everything else should be deprecated. |
I share Tom's surprise that this PR is arousing discomfort. But evidently it is, so we need some clear discussion as to why, and what to do about it. I think that Tom's rationale for it is quite clear. I am open to arguments against it, or to suggested modifications or alternatives. |
On 2015/09/03 7:51 PM, Thomas A Caswell wrote:
Sorry, I haven't been able to figure out what you mean here. Are some |
The suggested signature for the non implicit calls is func(ax, data, The trade off is partially consistent api between interactive/non On Fri, Sep 4, 2015, 02:22 Eric Firing notifications@github.com wrote:
|
I guess the concern comes down to the fact that right now this PR doesn't make the mpl codebase more succinct and it represents 4 new functions in the pyplot namespace which need consideration. If we want to iterate on this in this PR, rather than as a MEP, I'm happy to do that, but I suspect there will be several threads to the discussion which doesn't read well in a linear GH style... I'll outline some of my concerns, and we can go from there: ensure_ax.* decoratorsIt is unclear to me that a decorator is the way to go here:
Assuming the weird thing is a deal breaker, the problem is hugely simplified, and a function would do:
Truthfully though, the backwards compatible way is just as good:
The question about whether axes should be the first argument for all plotting functions needs to be also addressed. I completely understand the recommendation, but doing so means that you cannot then call the argument optional (without breaking the expected Python definition of positional). So I think we need to look at the motivation again - is the recommendation that axes be the first argument related to the fact that our current Axes methods are implicitly doing this (through self)? Or is there another reason why accepting ax as an optional argument is an unhealthy recommendation? The idea that developers wouldn't need pyplot (specifically Finally, if there are to be this many repetitively named items in pyplot, I'd consider giving them a namespace (via a class) - it would also allow for some of the duplicated code to be shared between functions. gna.* functionI'm all in favour of improving the interface - I appreciate we are where we are, but the Incidentally, I was surprised that only those two kwargs were added to the I hope this isn't coming across as negative - that is not my intention. I think there is room for debate here about the best course for us to be setting for matplotlib, and I'm very comfortable with @tacaswell leading that discussion; I just want to ensure we have it rather than take ad-hoc changes without an overarching strategy. |
On 2015/09/04 8:08 AM, Phil Elson wrote:
MEPs via GH have the same problem, don't they? Should we move this to
Very helpful, thank you. |
@tacaswell I understand that we can't merge these features between RC and 1.5, but that doesn't prevent us from releasing this in 2.1? I would rather see us get this right, than rush it. 👍 to what @pelson says regarding an overarching strategy and about the naming, I really dislike gcf, etcetera. it took me ages to realise what this meant having seen it many times sprawled throughout the code I saw it sprawled throughout the code... lets keep it readable. @efiring I think MEPs allow us to more easily discuss logic, by commenting on specific ideas/lines. Coincidently I have just pushed an MEP (#5029) to begin a conversation around the future of the axes class. Quite wide-ranging, but I don't think it will cover this area specifically, but it may pick up on a few of the points said here and add to the list of questions pertaining to this PR... |
Block quoting to make it clear what I am responding to, the gna discussion is simpler.
I didn't know the The point of Throwing it in as a top-level function seemed like a useful thing to do but I have no problem nuking it if we want to hold off on adding it for a more comprehensive simplification of this class of things (but I think it adds a useful functionality and does not make things that much worse). |
As excited as I am about this, I am leaning towards giving up on it for now 😞 I really want this to go in, but not over @pelson 's protests and I want to get 1.5rc1 tagged (maybe tomorrow night?) |
After phone call with @pelson we have decided to spin this off into it's own project. This lets us:
|
👍 |
Thanks to @tacaswell for being very gracious on this. I think the distinct package approach gives us some great opportunities, with very little cost (an extra installation). The big selling point for me was the ability for package developers to use these wrappers for older versions of matplotlib. 👍 to @tacaswell. 👏 |
Decorator for making user functions using the suggested signature
work with the state machine to plot the the implicit currently active
Axes.
This started from a discussion at lmfit/lmfit-py#146 (comment) and was mostly implemented by @danielballan at https://github.com/Nikea/xray-vision/blob/master/xray_vision/utils/mpl_helpers.py (which is for my day-job at BNL).
This is roughly related to my on-going attempts to simplify
pyplot
(see #3587, #2736 and #4091)This need a lot more documentation before they are merged, throwing this up to get feed back.