Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
BUG Ignore invisible axes in computing tight_layout#8244
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 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 :) |
| @@ -157,6 +158,15 @@ def test_tight_layout8(): | |||
| example_plot(ax, fontsize=24) | |||
| @image_comparison(baseline_images=['tight_layout9']) | |||
| def test_tight_layout9(): | |||
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.
How is this passing? The baseline images have not been added...
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.
Bear with me here, I have no idea howmatplotlib handles its tests. 😅 Was planning on getting back to this in a later commit, but you've caught me red-handed.
lib/matplotlib/tight_layout.py Outdated
| @@ -123,6 +123,9 @@ def auto_adjust_subplotpars(fig, renderer, | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 evercalls method does that filtering?
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.
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 ifall 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 aany to anall.
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.
grrr, I fail reading comprehension... you do have aall. But the filter should still be down below.
ResidentMarioMar 10, 2017 • 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.
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.
The first commit tried that approach (seehere).
But it actually failed to fix the issue. If you try to use that code with the original minimal failing example:
importmatplotlib.pyplotaspltf,axarr=plt.subplots(2,2)axarr[1][1].set_visible(False)plt.tight_layout()
Theunion op will fail because the list comprehension will pass it an empty list ([]).
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 haveget_tightbbox called on them, which would also fail. You need to protect that (my original approach),and protect from aunion() call on an empty list (your current approach).
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.
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? |
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.
Only concern would be if we really need the image comparison tests here. The bug it is fixing is 'do raise'.
Thanks@ResidentMario ! |
| @@ -157,6 +158,15 @@ def test_tight_layout8(): | |||
| example_plot(ax, fontsize=24) | |||
| @image_comparison(baseline_images=['tight_layout9']) | |||
| def test_tight_layout9(): | |||
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.
There are now two test functions in one file with the same name. So the first one gets clobbered by the second one.
WeatherGodMar 16, 2017 • 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.
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.
Sorry, please ignore. I misreadtest_tight_layout8 astest_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.