Skip to content

FigureCanvasAgg.renderer and FigureCanvasAgg.get_renderer have subtly different semantics #13099

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

Open
anntzer opened this issue Jan 4, 2019 · 5 comments
Labels
keep Items to be ignored by the “Stale” Github Action

Comments

@anntzer
Copy link
Contributor

anntzer commented Jan 4, 2019

FigureCanvasAgg.get_renderer is defined as

    def get_renderer(self, cleared=False):
        l, b, w, h = self.figure.bbox.bounds
        key = w, h, self.figure.dpi
        reuse_renderer = (hasattr(self, "renderer")
                          and getattr(self, "_lastKey", None) == key)
        if not reuse_renderer:
            self.renderer = RendererAgg(w, h, self.figure.dpi)
            self._lastKey = key
        elif cleared:
            self.renderer.clear()
        return self.renderer

i.e. (ignoring the cleared kwarg) if the figure bbox or dpi has changed, generate a new renderer.
Although this won't matter in most cases, it will in the figure saving routines as they manipulate the dpi, or (in GUI backends) when the figure is being resized.
In the codebase, it looks like canvas.renderer and canvas.get_renderer() are both used more or less indiscriminately and I doubt anyone took the difference between them into account.
It also makes it harder to write 3rd-party backends (cough cough) which have to provide exactly the same semantics if they want to be swappable to replace the Agg backend.

Given that a renderer whose size/dpi does not match the figure's doesn't really make sense, I think we should deprecate get_renderer() and make .renderer a property that self-replaces when the figure size/dpi changes (or vice-versa deprecate .renderer, but that seems worse on a backcompat code-churn PoV); or, if we really worry about performance, move the renderer-regenerating behavior to e.g. check_renderer_invalidation(). (The cleared=True behavior should also be split out to e.g. get_clean_renderer()).

See also #1852.

@timhoffm
Copy link
Member

timhoffm commented Jan 6, 2019

Sounds reasonable. No clear preference on property vs. method.

Is there a need for get_clean_renderer() at all. This feels like doing two things at once. You can always do

renderer = figure.canvas.get_renderer()
renderer.clean()

similar for the property variant.

@tacaswell tacaswell added this to the v3.1 milestone Jan 6, 2019
@tacaswell
Copy link
Member

Does this relate to #13021 at all?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2019

No(?)

@timhoffm timhoffm modified the milestones: v3.1.0, v3.2.0 Feb 15, 2019
@tacaswell tacaswell modified the milestones: v3.2.0, needs sorting Sep 10, 2019
@github-actions
Copy link

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 27, 2023
@anntzer anntzer added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels May 28, 2023
@oscargus
Copy link
Member

An update here is that the cleared argument was deprecated in 3.6 and will be removed in the upcoming 3.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Items to be ignored by the “Stale” Github Action
Projects
None yet
Development

No branches or pull requests

4 participants