Repository navigation
BUG Ignore invisible axes in computing tight_layout - #8244
Conversation
|
This only fails pep8: |
|
Gotcha. @tomspur Presumably I should add a test somewhere too, right? These CIs are taking five-ever... |
|
Yes, a test/picture helps to understand this issue and stops from breaking it again :) |
|
|
||
|
|
||
| @image_comparison(baseline_images=['tight_layout9']) | ||
| def test_tight_layout9(): |
There was a problem hiding this comment.
How is this passing? The baseline images have not been added...
There was a problem hiding this comment.
Bear with me here, I have no idea how matplotlib handles its tests. 😅 Was planning on getting back to this in a later commit, but you've caught me red-handed.
| for subplots, ax_bbox, (num1, num2) in zip(subplot_list, | ||
| ax_bbox_list, | ||
| num1num2_list): | ||
| if all([not ax.get_visible() for ax in subplots]): |
There was a problem hiding this comment.
shouldn't this just be filtering out invisible axes? subplots = [ax for ox in subplots if ax.get_visible()]
Would it make more sense to make sure what ever calls method does that filtering?
There was a problem hiding this comment.
yeah, I don't understand why you changed what I originally suggested. I suggested putting a filter in the list comprehension down below. Now, it does make sense to skip this iteration if all the axes in this group are invisible. So, I would put the filter in the list comprehension down below, and also change this one from a any to an all.
There was a problem hiding this comment.
grrr, I fail reading comprehension... you do have a all. But the filter should still be down below.
There was a problem hiding this comment.
The first commit tried that approach (see here).
But it actually failed to fix the issue. If you try to use that code with the original minimal failing example:
import matplotlib.pyplot as plt
f, axarr = plt.subplots(2, 2)
axarr[1][1].set_visible(False)
plt.tight_layout()The union op will fail because the list comprehension will pass it an empty list ([]).
There was a problem hiding this comment.
Before making a PR, please check that your commit actually fixes the issue...ahem. 😅
Skipping the laying calculations altogether (the approach here) meanwhile seems to work.
There was a problem hiding this comment.
Yes, that's why you need both approaches. if a subset of the subplots are not visible, then the way the code is currently, the invisible axes still have get_tightbbox called on them, which would also fail. You need to protect that (my original approach), and protect from a union() call on an empty list (your current approach).
There was a problem hiding this comment.
OK, I understand you now—I've re-added the conditional check.
|
attn @NelleV @anntzer @Kojoley Something is going wrong with pytest here. Running this locally I get: I would expect the three xfail to be real fails (due to missing files). |
|
This is the culprit (decorators.py, copy_baseline): I would just remove that chunk and replace by a real fail in the actual test function. |
|
See #8262 for a fix to my previous comment, sorry to have hi-jacked this PR with other issues. |
|
Always happy to be breaking things productively. 👍 |
|
This passes tests locally, except for an unrelated error in |
|
We have some flaky test, so we sometimes restart some tests by hand (hence travis rerunning magically :) ) |
|
All green. LGTM? |
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
Only concern would be if we really need the image comparison tests here. The bug it is fixing is 'do raise'.
|
Thanks @ResidentMario ! |
|
|
||
|
|
||
| @image_comparison(baseline_images=['tight_layout9']) | ||
| def test_tight_layout9(): |
There was a problem hiding this comment.
There are now two test functions in one file with the same name. So the first one gets clobbered by the second one.
There was a problem hiding this comment.
Sorry, please ignore. I misread test_tight_layout8 as test_tight_layout9. Really need to fix the default fonts on this computer...
BUG Ignore invisible axes in computing tight_layout
|
Backported to |
cf. #8225
Should pass tests. But, we'll see. Maybe this has side-effects.