Improve handling of subplots spanning multiple gridspec cells.#13544
Conversation
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
- The new
rowspan/colspanneed tests. - This needs an example of
label_outerI think, unless one already exists. But in particular for the more complicated labelling cases.
Not 100% convinced by the new API, but subplotspecs are kind of low level, so maybe slices are OK. I find slices a bit abstract versus lists.
| """update the subplot position from fig.subplotpars""" | ||
|
|
||
| self.figbox, self.rowNum, self.colNum, self.numRows, self.numCols = \ | ||
| self.figbox, _, _, self.numRows, self.numCols = \ |
There was a problem hiding this comment.
Why keep self.numRows\Cols around?
There was a problem hiding this comment.
Because I don't have a particularly good reason to deprecate them, whereas I'd say rowNum and colNum are actively encouraging people (... the few who'd touch that part of the codebase) to ignore the case of subplots spanning multiple cells.
|
rowspan and colspan are indirectly tested by the tests for label_outer: there's the one this PR adds, and the one just above it (test_shared). subplots_demo.py explicitly documents label_outer: |
| def colspan(self): | ||
| """The columns spanned by this subplot, as a `slice` object.""" | ||
| _, _, _, _, col_start, col_stop = self.get_rows_columns() | ||
| return slice(col_start, col_stop + 1) |
There was a problem hiding this comment.
get_rows_columns was just written as a helper (prob should have been private?, sorry!) Does it make more sense for this slice just to be an actual attribute that gets set at init, and get_rows_columns just uses that?
There was a problem hiding this comment.
The problem is that it is explicitly allowed to change a subplotspec's position in the grid by assigning to it (there's even a test for that!), so then you need to keep both num1/num2 and rowspan/colspan in sync, which just sounds overkill (it's not as if recomputing the rowspan and colspan every time was going to be a bottleneck...).
There was a problem hiding this comment.
Oh good lord, someone overthought that!
There was a problem hiding this comment.
I changed these 1) to not use get_rows_columns() as I'll just deprecate it in a later PR (especially if you suggest it should have been private :)) in favor of using rowspan/colspan, and 2) to return a range object rather than a slice object (the difference is small, but basically the range always has start and stop as integers (whereas a slice could have start or stop = None (corresponding to obj[:stop] or obj[start:]) or use negative values to mean "from the end", even though here I did normalize them to positive integers); plus, ranges support x in range and are hashable if we ever need that).
There was a problem hiding this comment.
That all seems reasonable to me.
anntzer
force-pushed
the
subplotspans
branch
2 times, most recently
from
cc8eb22 to
944a2d0
Compare
| for l in ax.get_xticklabels() + [ax.get_xaxis().offsetText]: | ||
| assert l.get_visible() == vx, \ | ||
| "X axis was incorrectly %s" % (tostr(vx)) | ||
| f"X axis #{i} is incorrectly {tostr(vx)}" |
There was a problem hiding this comment.
Would just go for
visibility = "invisible" if vx else "visible"
assert l.get_visible() == vx, \
"x-axis of Axes {i} was incorrectly {visibility}"
There was a problem hiding this comment.
but then you need to do the same for y. rephrased the thing.
| def test_label_outer_span(): | ||
| fig = plt.figure() | ||
| gs = fig.add_gridspec(3, 3) | ||
| # +-+-+-+ |
There was a problem hiding this comment.
# +---+---+---+
# | 1 | |
# +---+---+---+
# | | | 3 |
# + 2 +---+---+
# | | 4 | |
# +---+---+---+
I was a bit confused by the 2 in the condensed form.
anntzer
force-pushed
the
subplotspans
branch
3 times, most recently
from
dfae504 to
b7db9ce
Compare
anntzer
force-pushed
the
subplotspans
branch
from
b7db9ce to
f4412c0
Compare
|
This changed the |
|
Yes. Thanks for catching this error! |
4 participants
See changelog.
PR Summary
PR Checklist