Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use SubplotSpec row/colspans more, and deprecate get_rows_columns.#16618
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
This is pretty long to be considered just a deprecation. |
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.
Overall using the colspan and rowspan is nicer than what was being done before and helpful. There was one block in there where@anntzer had to obfuscate a relatively straightforward piece of code to the point that I can't really tell what it does for very little savings IMHO.
| pos, ax = pos_ax | ||
| return pos | ||
| minrow, minrow_ax = min( |
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.
This code block is pretty obfuscated compared to the original.
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.
fair enough, went for some compromise...
@QuLogic fair enough (though replacing get_rows_columns() everywhere is necessarily going to involve quite a few changes), do you have a suggested path forward here? |
| if location == 'right': | ||
| if col_stop <= maxcol: |
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.
@jklymak btw, are you really sure about whether the various inequalities here must be >/< or >=/<=? I'm not 100% sure they're all correct, and a comment explaining strict vs non-strict equalities may help...
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.
Well, you are the one who decided it should all be off by one, so I assumed you carefully checked ;-) It passes the tests so that is encouraging. There are lots of quite complex tests, so its hard to see how you broke something if it passes.
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 actually was referring to the original version.
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.
ha ha, OK. But again, the tests are pretty complex and don't fail in any weird ways. If these inequalities were not correct, its a little hard to see how the whole thing would work. But I can go through and try and remember why each is the way it is.
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 was slightly puzzled by them, so if you can add a comment that would be nice, but there's no urgency whatsoever.
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.
These are all correct. Lets say the parents of the colorbar span columns 1-3 of a six column layout (indexed 0, 1, 2, 3, 4, 5).maxrow is then 3. If a subplot span stops at 3 or less then the colorbar is to the right (I assume in this case thatsubspec.colspan.stop == 4). If the subplotstarts 4 or higher the colorbar is to its left.
Note its entirely possible for a subplot to not be positioned relative to the colorbar. i.e. if in another row it spans columns 0-5 that is perfectly allowed and neither stacking would happen. Saying that I wonder if Ido have a bug in here where I should only callhstack if order is not empty. But maybe this should go in first and I fix things like that.
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.
Hmm, I guess I'll just trust you on this for now and leave things as they are (with the mechanical translation) ;)
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 think thats fine, but I will go through and fix this after your translation is in.
QuLogic commentedMar 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just don't call it (the PR) a deprecation ;) The deprecation is a side effect of the re-factor, even if it may have been its initial cause. |
fair enough, edited the commit message :) |
Uh oh!
There was an error while loading.Please reload this page.
| height1 = heights[idx1] | ||
| # Horizontally align axes spines if they have the same min or max: | ||
| if not alignleft and colspan0.start == colspan1.start: | ||
| _log.debug('same start columns; line up layoutbox lefts') |
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.
Bit weird these debug messages, because they never mention whichax0 orax1 they're talking about.
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 assume these were intended to help debug the original implementation in simple cases where it's "clear" which axes are involved...
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
rebased |
Tests fail due to removed imports. |
and simplify constrained_layout by using rowspan/colspan instead.
(The deprecation was suggested in#13544 (comment).)
PR Summary
PR Checklist