Use SubplotSpec row/colspans more, and deprecate get_rows_columns.#16618
Conversation
|
This is pretty long to be considered just a deprecation. |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
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.
This code block is pretty obfuscated compared to the original.
There was a problem hiding this comment.
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? |
| nrows, ncols, row_start, row_stop, col_start, col_stop = \ | ||
| subspec.get_rows_columns() | ||
| if location == 'right': | ||
| if col_stop <= maxcol: |
There was a problem hiding this comment.
@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.
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.
I actually was referring to the original version.
There was a problem hiding this comment.
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.
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.
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 that subspec.colspan.stop == 4). If the subplot starts 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 I do have a bug in here where I should only call hstack 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.
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.
I think thats fine, but I will go through and fix this after your translation is in.
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. |
anntzer
changed the title
|
fair enough, edited the commit message :) |
| 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.
Bit weird these debug messages, because they never mention which ax0 or ax1 they're talking about.
There was a problem hiding this comment.
I assume these were intended to help debug the original implementation in simple cases where it's "clear" which axes are involved...
|
rebased |
|
Tests fail due to removed imports. |
3 participants
and simplify constrained_layout by using rowspan/colspan instead.
(The deprecation was suggested in #13544 (comment).)
PR Summary
PR Checklist