-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
There was a problem hiding this 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] |
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/matplotlib/_layoutgrid.py
Outdated
hc = [self.lefts[0] == 0, | ||
self.rights[-1] == 1, | ||
if not isinstance(parent, LayoutGrid): | ||
# specify a rectangle in figure co-ordinates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# specify a rectangle in figure co-ordinates | |
# specify a rectangle in figure coordinates |
There was a problem hiding this comment.
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!) ;-)
c39027a
to
acae2c4
Compare
@@ -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]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def make_layoutgrids(fig, layoutgrids, rect=[0, 0, 1, 1]): | |
def make_layoutgrids(fig, layoutgrids, rect=(0, 0, 1, 1)): |
There was a problem hiding this comment.
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...
lib/matplotlib/layout_engine.py
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[left, bottom, width, height], each from 0-1. | |
(left, bottom, width, height), each from 0-1. |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, needed to rebase.
lib/matplotlib/layout_engine.py
Outdated
rect : [l, b, w, h] | ||
Rectangle in figure coordinates to perform constrained layout in | ||
[left, bottom, width, height], each from 0-1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
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 Is there any reason why you deleted pytest.ini and tests.py here? |
No, must have been a rebase gone awry? Sorry I didn't notice that. |
No worries, I'll restore them. |
Looks like we have a new pytest.ini. |
What does test.py do? |
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. |
PR Summary
Closes #22623 adding a rectangle to constrained layout
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).