Skip to content

FIX constrained_layout w/ hidden axes #14919

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 12, 2019
Merged

FIX constrained_layout w/ hidden axes #14919

merged 2 commits into from
Aug 12, 2019

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jul 30, 2019 •
edited
Loading

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 jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Jul 30, 2019
@jklymak jklymak added this to the v3.1.2 milestone Jul 30, 2019
@jklymak
Copy link
Member Author

jklymak commented Jul 30, 2019

Could easily be back ported if not a bother.

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

ImportanceOfBeingErnest commented Jul 30, 2019 •
edited
Loading

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
Copy link
Member Author

jklymak commented Jul 30, 2019

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
Copy link
Member

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

@jklymak
Copy link
Member Author

jklymak commented Jul 30, 2019

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
timhoffm reviewed Aug 5, 2019
View reviewed changes
lib/matplotlib/_constrained_layout.py
@@ -272,7 +272,11 @@ def _make_layout_margins(ax, renderer, h_pad, w_pad):
invTransFig = fig.transFigure.inverted().transform_bbox
pos = ax.get_position(original=True)
tightbbox = ax.get_tightbbox(renderer=renderer)
bbox = invTransFig(tightbbox)
if tightbbox is None:
Copy link
Member

Choose a reason for hiding this comment

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

Just:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
dstansby approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 with a comment on the test

@jklymak jklymak force-pushed the fix-CL-hidden-axes branch from eb410f0 to 1911cc8 Compare August 6, 2019 15:23
@jklymak jklymak force-pushed the fix-CL-hidden-axes branch 3 times, most recently from 18538e2 to 47f0128 Compare August 8, 2019 15:23
@timhoffm
Copy link
Member

timhoffm commented Aug 8, 2019

flake8:

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

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

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

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

jklymak commented Aug 8, 2019

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 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
@anntzer
Backport PR matplotlib#14919: FIX constrained_layout w/ hidden axes
ab203bd
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 12, 2019
Merged
dstansby added a commit that referenced this pull request Aug 13, 2019
@dstansby
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm left review comments

@dstansby dstansby dstansby approved these changes

@anntzer anntzer anntzer approved these changes

@ImportanceOfBeingErnest 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
@jklymak @ImportanceOfBeingErnest @timhoffm @anntzer @dstansby