Only test png output for mplot3d. #13901
Conversation
|
Possibly worth mentioning in the commit message that the choice of png over pdf is for ease of diffing, despite size costs |
d65f30b to
a50c7ea
Compare
|
good point, edited the commit message accordingly |
| @@ -11,6 +13,9 @@ | |||
| import numpy as np | |||
|
|
|||
|
|
|||
| image_comparison = partial(image_comparison, extensions=['png']) | |||
There was a problem hiding this comment.
| image_comparison = partial(image_comparison, extensions=['png']) | |
| png_image_comparison = partial(image_comparison, extensions=['png']) |
That caught me by surprise. I was scolling past this and just saw the decorators with removed extensions parameter. Please use png_image_comparison or image_comparison_png. Having the same function name but with different API is a bit unclear; in particular also because we from ... import image_comparison in other tests.
a50c7ea to
016a8cb
Compare
| @@ -269,7 +260,7 @@ def test_text3d(): | |||
| ax.set_zlabel('Z axis') | |||
|
|
|||
|
|
|||
| @image_comparison(baseline_images=['trisurf3d'], remove_text=True, tol=0.03) | |||
| @image_comparison_png(baseline_images=['trisurf3d'], remove_text=True, tol=0.03) | |||
There was a problem hiding this comment.
E501 line too long (80 > 79 characters), here and in 5 other places
There was a problem hiding this comment.
Could fix by renaming to png_comparison
There was a problem hiding this comment.
indeed... should be fixed now
016a8cb to
4b05bcb
Compare
There was a problem hiding this comment.
Can you also remove the comment about the flaky svg tests (that are now gone, although that does suggest that we were exercising something funny about the SVG backend, however by gut guess is that it is related to the (previous lack of) stability of dictionary iteration).
|
Attn @WeatherGod |
4b05bcb to
16f67f9
Compare
|
@tacaswell edited accordingly |
|
I would prefer we wait for @WeatherGod to sign off on this. |
|
Probably my only reservation about dropping vector-based tests for mplot3d
is that there are some very odd bugs that do exist. The one off the top of
my head is the one where the panels come up the wrong colors. If I remember
correctly, it had something to do with alpha blending. So, not exactly a
mplot3d bug, but the rendered vector images aren't identical to the png
outputs.
The vector based tests has helped us before to notice in the vector
backends, but that might not be needed anymore.
…On Mon, Apr 8, 2019 at 9:28 PM Thomas A Caswell ***@***.***> wrote:
I would prefer we wait for @WeatherGod <https://github.com/WeatherGod> to
sign off on this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13901 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-PVxF9hvK1UbAnl_9I83ZgaYtXlWks5ve-zagaJpZM4cg_gY>
.
|
|
Can you point out which is the specifically problematic test? At least right now all vector files (that got deleted in this PR) appear identical to the corresponding png files (by visual inspection). |
|
So this can be shortened now with #14166? |
16f67f9 to
3a7dff3
Compare
|
indeed, done. |
mplot3d is not exercising any pdf or svg-specific code paths so we can just test pngs only, saving ~1.8Mb of baseline images (especially if we regenerate them). pngs were chosen instead of pdfs, despite being bigger, because they can be visually diffed on Github.
3a7dff3 to
e0e51f3
Compare
|
@tacaswell @timhoffm either of you wants to merge this? :) (both of you already approved) |
mplot3d is not exercising any pdf or svg-specific code paths so we can
just test pngs only, saving ~1.8Mb of baseline images (especially if we
regenerate them).
See discussion starting at #8896 (comment).
PR Summary
PR Checklist