Avoid using pyplot for check_figures_equal#31076
Avoid using pyplot for check_figures_equal#31076ayshih wants to merge 6 commits intomatplotlib:mainfrom
Conversation
f22fb3b to
4db42cf
Compare
|
It doesn't appear that any existing test needs these figures to be in the |
|
Cool! |
|
I'm not sure what's going on w/ coverage here but the failures are in related functions. |
jklymak
left a comment
There was a problem hiding this comment.
You never call func so the none of the code in any of the tests gets called.
Please test locally with a test that should fail and make sure it fails.
|
Ha ha, quite right |
4db42cf to
0e85fc7
Compare
|
I am very 👍🏻 on this change! |
6a732d8 to
d0ac853
Compare
|
It is probably worth adding a check that at least one of the figures has any marks. Means that "empty figure is empty" is not a test we can write, but I'm not super worried about preserving that possibility! |
d0ac853 to
9e5f156
Compare
|
Okay, check added |
lib/matplotlib/testing/decorators.py
Outdated
| fig_test = Figure() | ||
| fig_ref = Figure() | ||
| func(*args, fig_test=fig_test, fig_ref=fig_ref, **kwargs) | ||
| if not (fig_test.get_axes() or fig_ref.get_axes()): |
There was a problem hiding this comment.
It's not a requirement that we add Axes for a test; some tests may just draw text or something. For non-empty, you really need to check that all pixels are not white.
There was a problem hiding this comment.
the fig.get_children() == 1 check is good enough and much cheaper than checking every pixel. 👍
There was a problem hiding this comment.
There are two tests that check that an empty figure equals an empty figure, so I have modified those tests
9e5f156 to
5cc7fca
Compare
c1977d3 to
dd31993
Compare
dd31993 to
97612de
Compare
97612de to
8bc5453
Compare
| # TODO: Look into why the figures need to be pyplot figures to match | ||
| plt.figure(fig_ref) | ||
| plt.figure(fig_test) |
There was a problem hiding this comment.
I have no idea why this is necessary. Rendered figures using the Cairo backend are apparently slightly different depending on whether the figures are pyplot-managed or not. fig_ref and fig_test do not match when the figures are not pyplot-managed, but they do match when the figures are pyplot-managed.
For comparison, with the Agg backend instead, fig_ref and fig_test do not match regardless of whether the figures are pyplot-managed or not.
There was a problem hiding this comment.
Because when we do fig.savefig('foo.png') we look at the current canvas and ask "can you save a png?" if yes then we save the png using the current canvas. If we can not, then go look up a canvas that can save a png, swap the canvas on the figure, re-render to save the file, and then put the old canvas back.
There was a problem hiding this comment.
The @pytest.mark.backend('cairo') mark is picked up by the auto-fixture and sets the backend via plt.set_backend to be what ever backend is needed. If pyplot knows about the figures, then they get the cairo backend and use cairo to save the png. If they are "free" figures, then CanvasBase can not save a png so we select the Agg backend to render it which (for reasons I do not know but am guess are related to drawing collections aware of their parts or not) gives different behavior in this case.
Explictly creating and setting to a cairo canvas should also work.
There was a problem hiding this comment.
Ah, understood, I'll adjust my comment
|
Okay, ready for review again! |
| match=f'Data with more than {msg} cannot be ' | ||
| 'accurately displayed.'): | ||
| fig_test.canvas.draw() | ||
| with io.BytesIO() as buffer: |
There was a problem hiding this comment.
This is the same underlying problem as with cairo, because CanvasBase does not have the ability to draw anything (and if I recall this warning comes out of Agg) so this forces Agg to be used to render.
PR summary
The
@check_figures_equaltesting decorator currently usespyplotto create the two figures, but the use ofpyplotmeans that a wrapped test is automatically not thread-safe, even when the test itself does not need anypyplotfunctionality. This PR changes the testing decorator to create the twoFigureinstances directly, which means that such tests will not immediately fail when testing for thread safety (e.g., usingpytest-run-parallel). I also removed the check for whether any newpyplotfigures are created during the test because that is also not thread-safe.PR checklist