Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

TST: Use placeholders for text in layout tests#29872

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

Merged
timhoffm merged 4 commits intomatplotlib:mainfromQuLogic:layout-text-placeholders
Apr 10, 2025

Conversation

QuLogic
Copy link
Member

PR summary

This is an alternative to#29833 which patches theText 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 changingmatplotlib.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

@QuLogicQuLogic added topic: geometry managerLayoutEngine, Constrained layout, Tight layout topic: testing labelsApr 5, 2025
@github-actionsgithub-actionsbot removed the topic: geometry managerLayoutEngine, Constrained layout, Tight layout labelApr 5, 2025
@@ -641,7 +644,7 @@ def test_compressed1():
fig.draw_without_rendering()

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)
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Note, I foundtest_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.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good. I would never have known to use monkeypatch!

timhoffm
timhoffm previously requested changesApr 7, 2025
if self.get_text() == '':
return
bbox = self.get_window_extent()
Rectangle(bbox.p0, bbox.width, bbox.height, color='grey').draw(renderer)
Copy link
Member

@timhoffmtimhoffmApr 7, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
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.

@QuLogicQuLogicforce-pushed thelayout-text-placeholders branch fromde67704 to35442a2CompareApril 8, 2025 00:22
@QuLogic
Copy link
MemberAuthor

I pushed a couple minor tweaks:

  1. Changed the box to get its colour from the text. This isn't used in these tests, but could be elsewhere.
  2. Removed the edge colour from the box.
  3. Added a comment about the bounding box size.
  4. Tweaked the importing so that there's only one fixture.

@timhoffmtimhoffm dismissed theirstale reviewApril 8, 2025 02:13

Edge color removed

return
bbox = self.get_window_extent()
rect = Rectangle(bbox.p0, bbox.width, bbox.height,
facecolor=(self.get_color(), 0.5), edgecolor='none')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

timhoffm reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Test images looks a bit redacted now 😆, but I think this is better.

@QuLogic
Copy link
MemberAuthor

I made one more change to 2 tests that I found in#29816 that are about tight layout, namely that callingfig.tight_layout() within the test, withimage_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.

@QuLogicQuLogicforce-pushed thelayout-text-placeholders branch 2 times, most recently from37e5518 to064c10eCompareApril 8, 2025 10:20
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@QuLogicQuLogicforce-pushed thelayout-text-placeholders branch fromd46840e to7c466f9CompareApril 9, 2025 07:03
@github-actionsgithub-actionsbot added the Documentation: devdocsfiles in doc/devel labelApr 9, 2025
@QuLogic
Copy link
MemberAuthor

QuLogic commentedApr 9, 2025
edited
Loading

While updating that doc section, I was also reminded that we should prefermpl20 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

@timhoffm
Copy link
Member

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 istight_layout_offsetboxes2.svg

image

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.

@jklymak
Copy link
Member

Tight_layoutshould account for the actual reported label sizes.

@timhoffm
Copy link
Member

So is this an issue due the rectangle not representing the actual text width but rather an approximation?

@jklymak
Copy link
Member

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.

@timhoffm
Copy link
Member

timhoffm commentedApr 9, 2025
edited
Loading

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.

@QuLogic
Copy link
MemberAuthor

Tight_layoutshould account for the actual reported label sizes.

But these are not labels, they areAnchoredOffsetBox. 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 thereal test is the difference between 1 and 2, where the invisibleAnchoredOffsetBox 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?

@jklymak
Copy link
Member

Oh I see. Yes the boxes can overlap the labels in their own axes. They should not overlap labels in other axes.

@QuLogic
Copy link
MemberAuthor

Here's my suggestion for removing the image from the test:b6dff32

It creates 3 figures:

  1. one without anyAnchoredOffsetbox
  2. one withAnchoredOffsetbox around eachAxes
  3. one with them invisibleAnchoredOffsetbox

Then the test is that in figure 2, eachAxes 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.

@tacaswelltacaswell added this to thev3.10.2 milestoneApr 10, 2025
QuLogicand others added4 commitsApril 10, 2025 14:51
If we pass `remove_text` to `image_comparison`, we don't expect tests tochange from FreeType. But if `tight_layout` is called *before* the endof the test function, the layout will happen with the text that wassupposed to be removed.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell
Copy link
Member

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

@QuLogicQuLogicforce-pushed thelayout-text-placeholders branch from7c466f9 to3d0fd3cCompareApril 10, 2025 18:55
@timhoffmtimhoffm merged commit4658ab0 intomatplotlib:mainApr 10, 2025
41 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 10, 2025
@QuLogicQuLogic deleted the layout-text-placeholders branchApril 10, 2025 22:31
rcomer pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 20, 2025
rcomer added a commit that referenced this pull requestApr 20, 2025
…872-on-v3.10.xBackport PR#29872 on branch v3.10.x (TST: Use placeholders for text in layout tests)
@ksundenksunden mentioned this pull requestMay 9, 2025
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.10.3
Development

Successfully merging this pull request may close these issues.

4 participants
@QuLogic@timhoffm@jklymak@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp