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

FIX: maybe improve renderer dance by jklymak · Pull Request #22732 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX: maybe improve renderer dance - #22732

Merged
tacaswell merged 1 commit into
matplotlib:mainfrom
jklymak:fix-renderer-dance
Apr 4, 2022
ConversationCommits (1)ChecksFiles changed
Merged

FIX: maybe improve renderer dance#22732
tacaswell merged 1 commit into
matplotlib:mainfrom
jklymak:fix-renderer-dance

Conversation

jklymak commented Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Closes #22673

tight_layout and constrained_layout were both calling backend_bases._get_renderer. However, that was probably too low-level, and seems to cause canvas-dpi related flakiness as in #22673, in particular some users were seeing changed dpi because savefig.format was set to a vector backend.

This uses the more robust algorithm in _tight_layout.get_renderer. In particular if fig.canvas already exists and has a renderer, use that because it means the user has already (perhaps interactively) created a canvas. This stops prematurely falling back to a default canvas indicated by savefig.format. This probably mostly worked for folks because the default format is png which is enough like on-screen display that things didn't break.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • 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).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

jklymak mentioned this pull request Mar 30, 2022
Closed
jklymak added this to the v3.5.2 milestone Mar 30, 2022
jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Mar 30, 2022

jklymak commented Mar 30, 2022

Copy link
Copy Markdown
Member Author

BTW, I have no real way to test this, other than the existing tests, which pass....

jklymak marked this pull request as ready for review March 30, 2022 15:34

anntzer commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

I agree that this is better; likely this "use the cached renderer" logic should always be used even in backend_bases._get_renderer as long as print_method is None (i.e. we're not in a context of savefig with a known format)? That can be fixed lated, though.

tacaswell linked an issue Apr 4, 2022 that may be closed by this pull request
[Bug]: EPS savefig messed up by 'figure.autolayout' rcParam on 3.5.0 #21930
Closed
tacaswell approved these changes Apr 4, 2022
View reviewed changes

tacaswell 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

Seems a bit odd that everything is routed through tight_layout, but this closes at least 2 issues.

tacaswell merged commit 9e0eb54 into matplotlib:main Apr 4, 2022

lumberbot-app Bot commented Apr 4, 2022

Copy link
Copy Markdown

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.5.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 9e0eb54fd98bb804f51a74101db88164870a2458
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #22732: FIX: maybe improve renderer dance'
  1. Push to a named branch:
git push YOURFORK v3.5.x:auto-backport-of-pr-22732-on-v3.5.x
  1. Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #22732 on branch v3.5.x (FIX: maybe improve renderer dance)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

anntzer commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

Seems a bit odd that everything is routed through tight_layout

As noted above, probably that logic should be pulled out of TL and moved into backend_bases.

tacaswell commented Apr 4, 2022

Copy link
Copy Markdown
Member

This backport is odd because a bunch of stuff has moved around.....I'm doing it and will make sure the two linked issues still work.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 4, 2022
Backport PR matplotlib#22732: FIX: maybe improve renderer dance
c8c3767 
Merge pull request matplotlib#22732 from jklymak/fix-renderer-dance

FIX: improve cached renderer dance
(cherry picked from commit 9e0eb54)

The changes had to be moved to figure.py due to a major refactoring that will
be released in 3.6 that is already on main.
tacaswell mentioned this pull request Apr 4, 2022
Merged
jklymak deleted the fix-renderer-dance branch April 5, 2022 07:22

jklymak commented Apr 5, 2022

Copy link
Copy Markdown
Member Author

As noted above, probably that logic should be pulled out of TL and moved into backend_bases.

#22745 Moves the logic to Figure. The Figure holds the cache, so I'm not clear what we gain from putting the logic in the backends?

tacaswell added a commit that referenced this pull request Apr 5, 2022
Merge pull request #22783 from tacaswell/auto-backport-of-pr-22732-on…
0734a70 
…-v3.5.x

Backport PR #22732: FIX: maybe improve renderer dance

anntzer commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

It doesn't really matter to me whether figure or b_bases hold that logic (well, in my view figure is more just for artists and b_bases for canvas/renderer related stuff, but that's no big deal), mostly the point is that having that in t_l doesn't make much sense.

tacaswell mentioned this pull request Apr 13, 2022
Closed
imgbot Bot pushed a commit to kkipngenokoech/matplotlib that referenced this pull request Apr 19, 2026
Backport PR matplotlib#22732: FIX: maybe improve renderer dance
5252649 
Merge pull request matplotlib#22732 from jklymak/fix-renderer-dance

FIX: improve cached renderer dance
(cherry picked from commit 9e0eb54)

The changes had to be moved to figure.py due to a major refactoring that will
be released in 3.6 that is already on main.
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 approved these changes
anntzer anntzer approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: geometry manager LayoutEngine, Constrained layout, Tight layout

Projects

None yet

Milestone

v3.5.2

Development

Successfully merging this pull request may close these issues.

[Bug]: tight_layout (version 3.5+) [Bug]: EPS savefig messed up by 'figure.autolayout' rcParam on 3.5.0

4 participants

Back | FazBrowse Home | New Git URL