Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add a Figure._get_cachedRenderer() method#21319
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Yes I was wondering if it should try and get the rendere from the current figure if there is one in pyplot. |
| @@ -2378,6 +2378,13 @@ def axes(self): | |||
| get_axes = axes.fget | |||
| def _get_cachedRenderer(self, error_if_none=True): | |||
| # Get the cached renderer, raising an error if it doesn't exist yet | |||
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 guess ifself._cachedRenderer is None, I'd doself.draw_without_rendering() here and force a renderer (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.
This is probably a behaviour change and outside the scope of this (bugfix) PR? Perhaps not though, I'm not very well acquainted with renderers etc.
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.
We already have a public methodhttps://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.get_renderer_cache.htmlax.get_render_cache, do we want to prompt this to be public on the Figure too (or deprecate the one on Axes? We do not use it internally, it only shows up where it is defined, in an example, and in the source rst).
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 question, and I'm not entirely sure what the answer is as I'm not too sure why/if a user would want to get the cached renderer.
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 think for exactly the reason we are getting it here!
I can see an argument than all of the renderer cache stuff should be private. It is a cache and there is no good reason for the user to be poking around the cache, we want to be able to replace/clear it/etc at will. On the other hand, we expose a couple of public functions that take a renderer as an "optional" (but only because fill it in with the cache) argument. Making renderers is a bit finicky (and if you make it your self no guarantee that it is consistent with anything you care about!).
This comment has been minimized.
This comment has been minimized.
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 guess this needs an API change note as the Error has changed in both cases?
| @@ -2829,10 +2836,7 @@ def draw_artist(self, a): | |||
| This method can only be used after an initial draw of the figure, | |||
| because that creates and caches the renderer needed here. | |||
| """ | |||
| if self._cachedRenderer is None: | |||
| raise AttributeError("draw_artist can only be used after an " | |||
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 suspect that this is raisingAttributeError to maintain exact API compatibility with older versions of the code where_cachedRenderer was not an attribute until it was stashed 😱 .
I pushed this to 3.6 because I think this is a slightly bigger change than we want to put in a patch release, this is a very old bug, and there is multiple reasonable workarounds. |
I think this needs an API change note because the Exception type is changed. We may also want to do classNoRenderer(RuntimeError,AttributeError): ... and raise that instead (back-compatibleand a more informative name). |
@dstansby popped this to draft until the rebase and API change note are done. I imagine it is mergeable at that point? |
I don't have time to pick this up in the short term - if someone else wants to feel free |
I think so.... |
PR Summary
Adds
Figure._get_cachedRenderer()which can be used to get the cached renderer, but error if it's not present. This can be used in one existing place, and is also now used in a legend method, which probablyfixes#21285PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).