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

FIX constrained_layout w/ hidden axes by jklymak · Pull Request #14919 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX constrained_layout w/ hidden axes - #14919

Merged
anntzer merged 2 commits into
matplotlib:masterfrom
jklymak:fix-CL-hidden-axes
Aug 12, 2019
ConversationCommits (2)ChecksFiles changed
Merged

FIX constrained_layout w/ hidden axes#14919
anntzer merged 2 commits into
matplotlib:masterfrom
jklymak:fix-CL-hidden-axes

Conversation

jklymak commented Jul 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

If an axes is hidden (set_visible(False)) then it has no tightbbox. But we still need to position it w/ constrained_layout, pointed out in #14917 (thanks @ImportanceOfBeingErnest )

Closes #14918

import numpy as np
import matplotlib.pyplot as plt

fig5, axs = plt.subplots(2, 2, constrained_layout=True)
axs[0,1].set_visible(False)
plt.show()

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Jul 30, 2019
jklymak added this to the v3.1.2 milestone Jul 30, 2019

jklymak commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

Could easily be back ported if not a bother.

jklymak mentioned this pull request Jul 30, 2019
Merged
5 tasks done
jklymak force-pushed the fix-CL-hidden-axes branch from 56ecc84 to eb410f0 Compare July 30, 2019 03:51

ImportanceOfBeingErnest commented Jul 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Should a non-visible axes take part in the layout? If the answer to that is yes, this is a good fix; but one might also consider that a non-visible axes may just be treated as "not there", or maybe as the most tiny box possible at the center of its original position?

Forget the above. Yes, this is the correct fix. If you want to not have the axes take part in the layout you should use .remove() instead.

jklymak commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

An empty gridspec box gets a placeholder like this anyhow, so this is the smallest box that is possible for this spot if gridspec or subplots has been used for the layout.

I do this because if someone has a gap in their layout presumably they want it to be there. And it is needed to keep the axes in rows above or below a blank (for instance) aligned.

This is somewhat poorly documented at: https://matplotlib.org/3.1.1/tutorials/intermediate/constrainedlayout_guide.html#empty-gridspec-slots
and its checked in one of the tests... (the horribly named: test_constrained_layout8)

ImportanceOfBeingErnest commented Jul 30, 2019

Copy link
Copy Markdown
Member

Yes, I realized that 5 minutes after I posted (see edit above), sorry for the confusion.

jklymak commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

Too fast! Though I'll point out that even remove will still put a blank gridspec in there (actually an axes with all the elements made invisible!)

timhoffm reviewed Aug 5, 2019
View reviewed changes
Comment thread lib/matplotlib/_constrained_layout.py
pos = ax.get_position(original=True)
tightbbox = ax.get_tightbbox(renderer=renderer)
bbox = invTransFig(tightbbox)
if tightbbox is None:

timhoffm Aug 5, 2019

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

Just:

bbox = pos if tightbbox is None else invTransFig(tightbbox)

jklymak Aug 6, 2019

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

I find those really hard to read a lot of the time.

timhoffm Aug 9, 2019

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

Actually, when reading a function for a basic understanding what it does I find this more easy.

It's just one line saying: "Sets bbox depending on some condition". The if/else is already more low level.

But not fighting over this.

dstansby approved these changes Aug 6, 2019
View reviewed changes

dstansby 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

👍 with a comment on the test

Comment thread lib/matplotlib/tests/test_constrainedlayout.py
jklymak force-pushed the fix-CL-hidden-axes branch from eb410f0 to 1911cc8 Compare August 6, 2019 15:23
dstansby reviewed Aug 6, 2019
View reviewed changes
Comment thread lib/matplotlib/tests/test_constrainedlayout.py Outdated
Comment thread lib/matplotlib/tests/test_constrainedlayout.py Outdated
jklymak force-pushed the fix-CL-hidden-axes branch 3 times, most recently from 18538e2 to 47f0128 Compare August 8, 2019 15:23

timhoffm commented Aug 8, 2019

Copy link
Copy Markdown
Member

flake8:

./lib/matplotlib/_constrained_layout.py:279:1: W293 blank line contains whitespace

jklymak added 2 commits August 8, 2019 14:18
FIX: let CL work with hidden axes
f215d09
TST: test hidden axes
c8ec3c6 
Update lib/matplotlib/tests/test_constrainedlayout.py

Co-Authored-By: David Stansby <dstansby@gmail.com>

FLAKE8
jklymak force-pushed the fix-CL-hidden-axes branch from dc6186f to c8ec3c6 Compare August 8, 2019 21:19

jklymak commented Aug 8, 2019

Copy link
Copy Markdown
Member Author

Hmmm, I kept trying to rebase that change in, but I suspect I added the space in the "TST" commit, so it kept overwriting it. Ooops....

anntzer merged commit 76afe83 into matplotlib:master Aug 12, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 12, 2019
Backport PR matplotlib#14919: FIX constrained_layout w/ hidden axes
ab203bd
meeseeksmachine mentioned this pull request Aug 12, 2019
Merged
dstansby added a commit that referenced this pull request Aug 13, 2019
Merge pull request #15047 from meeseeksmachine/auto-backport-of-pr-14…
4a4980b 
…919-on-v3.1.x

Backport PR #14919 on branch v3.1.x (FIX constrained_layout w/ hidden axes)
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

timhoffm timhoffm left review comments
dstansby dstansby approved these changes
anntzer anntzer approved these changes
ImportanceOfBeingErnest ImportanceOfBeingErnest approved these changes

Assignees

No one assigned

Labels

topic: geometry manager LayoutEngine, Constrained layout, Tight layout

Projects

None yet

Milestone

v3.1.2

Development

Successfully merging this pull request may close these issues.

constrained_layout fails with hidden axis...

5 participants

Back | FazBrowse Home | New Git URL