Skip to content

ENH: rect for constrained_layout #22627

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

Merged
merged 1 commit into from
May 8, 2022
Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Mar 9, 2022

PR Summary

Closes #22623 adding a rectangle to constrained layout

import matplotlib.pyplot as plt
import numpy as np
import matplotlib as mpl

fig, ax = plt.subplots(layout='constrained')
fig.get_layout_engine().set(rect=[0.1, 0.1, 0.5, 0.5])
plt.show()

TestCLRect

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 jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Mar 9, 2022
@jklymak jklymak added this to the v3.6.0 milestone Mar 9, 2022
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -87,14 +87,18 @@ def do_constrained_layout(fig, h_pad, w_pad,
of 0.1 of the figure width between each column.
If h/wspace < h/w_pad, then the pads are used instead.

rect : [l, b, w, h]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor suggestion for the inputs here.

Suggested change
rect : [l, b, w, h]
rect : tuple of 4 floats

This should be the type, you're already specifying what they are below.

Also, do you want to change all the default inputs to tuples instead of a list, rect=(0, 0, 1, 1) to avoid mutability concerns?

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'm fine to do this, but note that we don't do this elsewhere in the library when we specify rectangles?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it seems rects are specified in a lot of different ways!

rect : tuple[float, float, float, float], optional
rect : sequence of float
rect : tuple (left, bottom, right, top), default: (0, 0, 1, 1)
rect : tuple of 4 floats, default: (0, 0, 1, 1)
rect : [left, bottom, width, height]
rect : (float, float, float, float)

I'm pretty indifferent on most of those styles, but I don't think the orientation belongs on this first line. My approval still stands regardless of which format of documentation you choose, consolidation of rect across matplotlib should be a follow-up issue.

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 changed this, though this is a private module.

hc = [self.lefts[0] == 0,
self.rights[-1] == 1,
if not isinstance(parent, LayoutGrid):
# specify a rectangle in figure co-ordinates
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# specify a rectangle in figure co-ordinates
# specify a rectangle in figure coordinates

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, though its a private module, and I like the British spelling better (at least I don't add an umlaut!) ;-)

@jklymak jklymak force-pushed the enh-rect-for-CL branch 3 times, most recently from c39027a to acae2c4 Compare March 11, 2022 05:12
@@ -133,7 +137,7 @@ def do_constrained_layout(fig, h_pad, w_pad,
return layoutgrids


def make_layoutgrids(fig, layoutgrids):
def make_layoutgrids(fig, layoutgrids, rect=[0, 0, 1, 1]):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def make_layoutgrids(fig, layoutgrids, rect=[0, 0, 1, 1]):
def make_layoutgrids(fig, layoutgrids, rect=(0, 0, 1, 1)):

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 guess the immutability is important enough to guard here...

@@ -196,15 +197,20 @@ def __init__(self, *, h_pad=None, w_pad=None,
If h/wspace < h/w_pad, then the pads are used instead.
Default to :rc:`figure.constrained_layout.hspace` and
:rc:`figure.constrained_layout.wspace`.
rect : tuple of 4 floats
Rectangle in figure coordinates to perform constrained layout in
[left, bottom, width, height], each from 0-1.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[left, bottom, width, height], each from 0-1.
(left, bottom, width, height), each from 0-1.

@@ -87,14 +87,18 @@ def do_constrained_layout(fig, h_pad, w_pad,
of 0.1 of the figure width between each column.
If h/wspace < h/w_pad, then the pads are used instead.

rect : tuple of 4 floats
Rectangle in figure coordinates to perform constrained layout in
[left, bottom, width, height], each from 0-1.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[left, bottom, width, height], each from 0-1.
(left, bottom, width, height), each from 0-1.

Copy link
Member

Choose a reason for hiding this comment

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

Should one possibly add default values here? (And in other locations.)

Copy link
Member

Choose a reason for hiding this comment

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

Btw, doesn't look like there is a renderer parameter (anymore?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, needed to rebase.

Comment on lines 252 to 254
rect : [l, b, w, h]
Rectangle in figure coordinates to perform constrained layout in
[left, bottom, width, height], each from 0-1.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rect : [l, b, w, h]
Rectangle in figure coordinates to perform constrained layout in
[left, bottom, width, height], each from 0-1.
rect : tuple of 4 floats
Rectangle in figure coordinates to perform constrained layout in
(left, bottom, width, height), each from 0-1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed all these, also in TightLayoutEngine..

@jklymak jklymak requested a review from oscargus May 8, 2022 17:52
@oscargus oscargus merged commit 9537906 into matplotlib:main May 8, 2022
@anntzer
Copy link
Contributor

anntzer commented May 27, 2022

@jklymak Is there any reason why you deleted pytest.ini and tests.py here?

@jklymak
Copy link
Member Author

jklymak commented May 27, 2022

No, must have been a rebase gone awry? Sorry I didn't notice that.

@anntzer
Copy link
Contributor

anntzer commented May 27, 2022

No worries, I'll restore them.

@jklymak
Copy link
Member Author

jklymak commented May 27, 2022

Looks like we have a new pytest.ini.

@jklymak
Copy link
Member Author

jklymak commented May 27, 2022

What does test.py do?

@jklymak jklymak deleted the enh-rect-for-CL branch May 27, 2022 10:07
@anntzer
Copy link
Contributor

anntzer commented May 27, 2022

It's been discussed a couple of times, e.g. at #20586. I think we should probably get rid of it at some point, but there still seems to be some users for now so that should be discussed separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: support rect with constrained_layout ("layout only to part of the figure")
4 participants