-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Only test png output for mplot3d. #13901
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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.
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.
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.
sounds good, fixed
a50c7ea to
016a8cb
Compare
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.
E501 line too long (80 > 79 characters), here and in 5 other places
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.
Could fix by renaming to png_comparison
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.
indeed... should be fixed now
016a8cb to
4b05bcb
Compare
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.
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.
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).
The test coverage doesn't drop, suggesting that all previously tested code paths in the vector backends are still being exercised.
If there are really some tricky points in the vector output, it would be better if they were exercised by a 2D plot rather than as a side effect by a 3D test, anyways...
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