TST: Use placeholders for text in layout tests - #29872
Conversation
Sorry, something went wrong.
|
|
||
| pos = axs[0, 0].get_position() | ||
| np.testing.assert_allclose(pos.x0, 0.2344, atol=1e-3) | ||
| np.testing.assert_allclose(pos.x0, 0.2381, atol=1e-2) |
There was a problem hiding this comment.
Note, I found test_compressed1 to be a bit flaky. Out of 200 repeats, 12 failed on this line or one of the two below where I changed the tolerance. I'm not entirely sure why, as the other tests don't have an issue. If you prefer, I can change the tolerance on all of these lines for consistency.
Sorry, something went wrong.
jklymak
left a comment
There was a problem hiding this comment.
Looks good. I would never have known to use monkeypatch!
Sorry, something went wrong.
| if self.get_text() == '': | ||
| return | ||
| bbox = self.get_window_extent() | ||
| Rectangle(bbox.p0, bbox.width, bbox.height, color='grey').draw(renderer) |
There was a problem hiding this comment.
| Rectangle(bbox.p0, bbox.width, bbox.height, color='grey').draw(renderer) | |
| Rectangle(bbox.p0, bbox.width, bbox.height, facecolor='grey').draw(renderer) |
color would also set the edgecolor, which is transparent by default ('none'). The edgecolor induces a line to be drawn around the region and since lines have antialiasing, the edges can become blury. We don't want that. See #29874 (comment), where the same effect shows up in a different context.
Sorry, something went wrong.
|
I pushed a couple minor tweaks:
|
Sorry, something went wrong.
| return | ||
| bbox = self.get_window_extent() | ||
| rect = Rectangle(bbox.p0, bbox.width, bbox.height, | ||
| facecolor=(self.get_color(), 0.5), edgecolor='none') |
There was a problem hiding this comment.
I'm a bit torn on using transparency here. On the one hand, that correctly catches overlay (but that should be rare). On the other hand, alpha blending is an additional component determining the output image, can that lead to more unstable reference images in the future?
Sorry, something went wrong.
There was a problem hiding this comment.
Fair enough. I think it should only make a difference if there is overlap and the z-order changes, but that's not important enough to check here.
Sorry, something went wrong.
There was a problem hiding this comment.
Test images looks a bit redacted now 😆, but I think this is better.
Sorry, something went wrong.
|
I made one more change to 2 tests that I found in #29816 that are about tight layout, namely that calling fig.tight_layout() within the test, with image_comparison(..., remove_text=True) does not layout correctly. The text would be removed after the layout, and thus the test image was still beholden to text properties. I fixed this by setting the layout on the Figure itself, so it would be enabled on save and correctly ignore text. Also, I changed the table test to use placeholders. |
Sorry, something went wrong.
timhoffm
left a comment
There was a problem hiding this comment.
This should also be mentioned in
https://matplotlib.org/devdocs/devel/testing.html#writing-an-image-comparison-test
Sorry, something went wrong.
|
While updating that doc section, I was also reminded that we should prefer mpl20 style. I guess I should update all of these, though it would change a bunch of figure sizes and also where the ticks are, so I'm not sure if I should for the layouting tests. That change would be here: https://github.com/QuLogic/matplotlib/compare/layout-text-placeholders..ltp |
Sorry, something went wrong.
|
I would do the mpl20 change in one go. I checked the diffs: The smaller figure sizes are ok. AFAICS no layout information is lost. The only one I'm not sure about is tight_layout_offsetboxes2.svg
The offset boxes (red/green) and the labels of the right column overlap. But I guess that's just how tightlayout works. It does not account for the actual label sizes. |
Sorry, something went wrong.
|
Tight_layout should account for the actual reported label sizes. |
Sorry, something went wrong.
|
So is this an issue due the rectangle not representing the actual text width but rather an approximation? |
Sorry, something went wrong.
|
I'm not sure - I dont' see an equivalent test for constrained layout - but maybe it is a draw twice sort of error, where the offset boxes get moved when the axes gets resized, and therefore the margin made for the axes is too small for th new position. We run constrained layout twice to get around that; usually the position is "closer" after the first time and the new margin is big enough. |
Sorry, something went wrong.
|
Hm @QuLogic maybe the workaround here could be to increase the figure size to not be bothered with this topic right now? 🤯 Not great, but digging into whether this is a tightlayout issue or a box approximation issue also does not feel like a good use of time. And I don't like the present image with the overlap. |
Sorry, something went wrong.
But these are not labels, they are AnchoredOffsetBox. Trying it out with constrained layout, it also doesn't seem to interact with the ticks either. Based on the comment in the test, it appears the intention is to ensure that the boxes are included in the Axes bounding box, but not necessarily have any effect on the ticks labels. Then the real test is the difference between 1 and 2, where the invisible AnchoredOffsetBox don't count for the bounding box in the latter, and it looks like that property is preserved. So I think that a) we can drop this test down to just .png since it's really just checking if a specific artist is included, b) maybe we should remove the tick labels entirely to avoid confusing ourselves, and c) maybe this doesn't need to be an image test at all? |
Sorry, something went wrong.
|
Oh I see. Yes the boxes can overlap the labels in their own axes. They should not overlap labels in other axes. |
Sorry, something went wrong.
|
Here's my suggestion for removing the image from the test: b6dff32 It creates 3 figures:
Then the test is that in figure 2, each Axes is smaller than in figure 1, and no boxes overlap. And then for figure 3, it's that it is the same as figure 1. If that makes some sense, then I'll rebase this PR on top of that, so we can drop the figures before changing them here. |
Sorry, something went wrong.
If we pass `remove_text` to `image_comparison`, we don't expect tests to change from FreeType. But if `tight_layout` is called *before* the end of the test function, the layout will happen with the text that was supposed to be removed.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
I am 👍 on moving to the version of that test that @QuLogic has proposed and moving all of these to mpl20 style in this PR. I think that this is the next PR in the sequence to move to freetype 2.13 |
Sorry, something went wrong.
…872-on-v3.10.x Backport PR #29872 on branch v3.10.x (TST: Use placeholders for text in layout tests)
PR summary
This is an alternative to #29833 which patches the Text object to draw a rectangle and not depend on any properties of the actual font. The calculation for the placeholders is somewhat arbitrary, but actually ends up rather close to the size of the original text, so most images don't change a whole lot.
I tested this by changing matplotlib.testing.set_font_settings_for_testing from DejaVu Sans to Noto Sans; this would have failed most of the tests before, but none failed now.
PR checklist