Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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 :tupleof4floats |
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], optionalrect : sequence of floatrect : 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 ofrect 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 figureco-ordinates | |
| # specify a rectangle in figurecoordinates |
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 toacae2c4Compare| @@ -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.
| defmake_layoutgrids(fig,layoutgrids,rect=[0,0,1,1]): | |
| defmake_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],eachfrom0-1. | |
| (left,bottom,width,height),eachfrom0-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],eachfrom0-1. | |
| (left,bottom,width,height),eachfrom0-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] | |
| Rectangleinfigurecoordinatestoperformconstrainedlayoutin | |
| [left,bottom,width,height],eachfrom0-1. | |
| rect :tupleof4floats | |
| Rectangleinfigurecoordinatestoperformconstrainedlayoutin | |
| (left,bottom,width,height),eachfrom0-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 inTightLayoutEngine..
@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
pytestpasses).flake8-docstringsand 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).