FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add a Figure._get_cachedRenderer() method by dstansby · Pull Request #21319 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Add a Figure._get_cachedRenderer() method - #21319

Closed
dstansby wants to merge 1 commit into
matplotlib:mainfrom
dstansby:cahced-render
ConversationCommits (1)ChecksFiles changed
Closed

Add a Figure._get_cachedRenderer() method#21319
dstansby wants to merge 1 commit into
matplotlib:mainfrom
dstansby:cahced-render

Conversation

dstansby commented Oct 8, 2021

Copy link
Copy Markdown
Member

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 probably fixes #21285

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak commented Oct 8, 2021

Copy link
Copy Markdown
Member

Yes I was wondering if it should try and get the rendere from the current figure if there is one in pyplot.

jklymak reviewed Oct 12, 2021
View reviewed changes
Comment thread lib/matplotlib/figure.py
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

jklymak Oct 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess if self._cachedRenderer is None, I'd do self.draw_without_rendering() here and force a renderer (I think).

dstansby Oct 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

tacaswell Dec 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We already have a public method https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.get_renderer_cache.html ax.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).

dstansby Dec 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

tacaswell Dec 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!).

dstansby added this to the v3.5.1 milestone Oct 17, 2021

This comment has been minimized.

Sign in to view
jklymak approved these changes Nov 29, 2021
View reviewed changes

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess this needs an API change note as the Error has changed in both cases?

tacaswell modified the milestones: v3.5.1, v3.6.0 Dec 1, 2021
tacaswell reviewed Dec 1, 2021
View reviewed changes
Comment thread lib/matplotlib/figure.py
because that creates and caches the renderer needed here.
"""
if self._cachedRenderer is None:
raise AttributeError("draw_artist can only be used after an "

tacaswell Dec 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I suspect that this is raising AttributeError to maintain exact API compatibility with older versions of the code where _cachedRenderer was not an attribute until it was stashed 😱 .

tacaswell commented Dec 1, 2021

Copy link
Copy Markdown
Member

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.

tacaswell commented Jan 26, 2022

Copy link
Copy Markdown
Member

I think this needs an API change note because the Exception type is changed. We may also want to do

class NoRenderer(RuntimeError, AttributeError):
    ...

and raise that instead (back-compatible and a more informative name).

jklymak marked this pull request as draft February 1, 2022 11:59

jklymak commented Feb 1, 2022

Copy link
Copy Markdown
Member

@dstansby popped this to draft until the rebase and API change note are done. I imagine it is mergeable at that point?

dstansby commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

I don't have time to pick this up in the short term - if someone else wants to feel free

jklymak mentioned this pull request Mar 31, 2022
Merged
6 tasks
timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022

QuLogic commented Jul 5, 2022

Copy link
Copy Markdown
Member

@jklymak #22745 added Figure._get_renderer, so can this be closed?

jklymak commented Jul 6, 2022

Copy link
Copy Markdown
Member

I think so....

jklymak closed this Jul 6, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
jklymak jklymak approved these changes

Assignees

No one assigned

Labels

status: needs documentation status: needs rebase status: orphaned PR status: superseded

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

legend.get_window_extent() fails

5 participants

Back | FazBrowse Home | New Git URL