Repository navigation
BUG Ignore invisible axes in computing tight_layout - #8244
NelleV merged 4 commits into
Conversation
Sorry, something went wrong.
|
This only fails pep8: |
Sorry, something went wrong.
|
Gotcha. @tomspur Presumably I should add a test somewhere too, right? These CIs are taking five-ever... |
Sorry, something went wrong.
|
Yes, a test/picture helps to understand this issue and stops from breaking it again :) |
Sorry, something went wrong.
|
|
||
|
|
||
| @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...
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 ([]).
Sorry, something went wrong.
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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
OK, I understand you now—I've re-added the conditional check.
Sorry, something went wrong.
|
attn @NelleV @anntzer @Kojoley Something is going wrong with pytest here. Running this locally I get: 15:47 $ python -m pytest -vv lib/matplotlib/tests/test_tightlayout.py ========================================================================================================================================================================================================= test session starts ========================================================================================================================================================================================================== platform linux -- Python 3.6.0, pytest-3.0.6, py-1.4.32, pluggy-0.4.0 -- /tmp/mpl_stock/bin/python cachedir: .cache rootdir: /home/tcaswell/source/p/matplotlib, inifile: pytest.ini collected 36 items lib/matplotlib/tests/test_tightlayout.py::test_tight_layout1[0-tight_layout1-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout1[0-tight_layout1-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout1[0-tight_layout1-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout2[0-tight_layout2-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout2[0-tight_layout2-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout2[0-tight_layout2-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout3[0-tight_layout3-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout3[0-tight_layout3-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout3[0-tight_layout3-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout4[0-tight_layout4-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py XPASS lib/matplotlib/tests/test_tightlayout.py::test_tight_layout4[0-tight_layout4-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py XPASS lib/matplotlib/tests/test_tightlayout.py::test_tight_layout4[0-tight_layout4-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py XPASS lib/matplotlib/tests/test_tightlayout.py::test_tight_layout5[0-tight_layout5-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout5[0-tight_layout5-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout5[0-tight_layout5-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout6[0-tight_layout6-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout6[0-tight_layout6-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout6[0-tight_layout6-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout7[0-tight_layout7-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout7[0-tight_layout7-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout7[0-tight_layout7-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout8[0-tight_layout8-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout8[0-tight_layout8-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout8[0-tight_layout8-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout9[0-tight_layout9-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py xfail lib/matplotlib/tests/test_tightlayout.py::test_tight_layout9[0-tight_layout9-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py xfail lib/matplotlib/tests/test_tightlayout.py::test_tight_layout9[0-tight_layout9-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py xfail lib/matplotlib/tests/test_tightlayout.py::test_outward_ticks[0-outward_ticks-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_outward_ticks[0-outward_ticks-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_outward_ticks[0-outward_ticks-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[0-tight_layout_offsetboxes1-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[0-tight_layout_offsetboxes1-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[0-tight_layout_offsetboxes1-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[1-tight_layout_offsetboxes2-png] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[1-tight_layout_offsetboxes2-pdf] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED lib/matplotlib/tests/test_tightlayout.py::test_tight_layout_offsetboxes[1-tight_layout_offsetboxes2-svg] <- /home/tcaswell/source/p/matplotlib/lib/matplotlib/tests/test_tightlayout.py PASSED =========================================================================================================================================================================================== 30 passed, 3 xfailed, 3 xpassed in 10.63 seconds =========================================================================================================================================================================================== I would expect the three xfail to be real fails (due to missing files). |
Sorry, something went wrong.
|
This is the culprit (decorators.py, copy_baseline): if os.path.exists(orig_expected_fname):
shutil.copyfile(orig_expected_fname, expected_fname)
else:
reason = ("Do not have baseline image {0} because this "
"file does not exist: {1}".format(expected_fname,
orig_expected_fname))
if is_called_from_pytest():
import pytest
pytest.xfail(reason)
else:
from ._nose import knownfail
knownfail(reason)
return expected_fname
I would just remove that chunk and replace by a real fail in the actual test function. |
Sorry, something went wrong.
|
See #8262 for a fix to my previous comment, sorry to have hi-jacked this PR with other issues. |
Sorry, something went wrong.
|
Always happy to be breaking things productively. 👍 |
Sorry, something went wrong.
|
This passes tests locally, except for an unrelated error in tight_layout8. A different unrelated failure appears to occur on travis (which reset and is now running again for some reason?). |
Sorry, something went wrong.
|
We have some flaky test, so we sometimes restart some tests by hand (hence travis rerunning magically :) ) |
Sorry, something went wrong.
|
All green. LGTM? |
Sorry, something went wrong.
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'.
Sorry, something went wrong.
|
|
||
|
|
||
| @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.
Sorry, something went wrong.
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...
Sorry, something went wrong.
BUG Ignore invisible axes in computing tight_layout
cf. #8225
Should pass tests. But, we'll see. Maybe this has side-effects.