Skip to content

TST: remove (most) text from constrained layout tests #29833

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

Closed
wants to merge 1 commit into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Mar 29, 2025

This removes most of the text from test_constrainedlayout.py. Rectangles are used to replicate x- y-labels and titles. Some ticks labels are just removed.

The only exception here are a couple of tests with suptitles. These get a special place in constrained layout, and I didn't see an easy way to put a rectangle in there instead (I'm sure its possible but I don't think the goal is to have zero text tests).

A follow up may be to remove many of the image tests, but really it is easier to see if things are working with an image test, so lets leave those in for now.

@jklymak jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout topic: testing labels Mar 29, 2025
@jklymak jklymak force-pushed the tst-constr-remove-text branch from 566a1c3 to 8563efa Compare March 29, 2025 23:10
@github-actions github-actions bot removed the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Mar 29, 2025
@jklymak jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Mar 29, 2025
@jklymak
Copy link
Member Author

jklymak commented Mar 29, 2025

This PR has flake8 errors like:

./lib/matplotlib/_mathtext.py:1613:9: F824 `nonlocal off_h` is unused: name is never assigned in scope
...

But clearly doesn't touch that file, so I think maybe flake8 is on a new version?

EDIT: ah yes 7.2 was released 4 hours ago.

@jklymak jklymak force-pushed the tst-constr-remove-text branch from 8563efa to 3909ae1 Compare March 30, 2025 21:44
@github-actions github-actions bot removed the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Mar 30, 2025
@jklymak
Copy link
Member Author

jklymak commented Mar 31, 2025

Sigh. Why does the bot remove tags that I have specifically added?

@timhoffm
Copy link
Member

You are also removing tick labels. Is that ok because they are trivial for the layout, or should they also be repaced with boxes?
image

@timhoffm
Copy link
Member

Could we just moneypatch Text.draw to draw a rectangle instead of characters?

If you want to be selective we could create a PlaceholderText class deriving from Text but drawing a rectangle instead. We could then switch out the class on an per-instance basis (artist.__class__ = PlaceholderText).

@jklymak
Copy link
Member Author

jklymak commented Mar 31, 2025

drawing text has a bunch of options (rotation, cantering etc) that seem like a lot of work to implement versus just redefining x/ylabel which does 95% of what we want.

@timhoffm
Copy link
Member

Are there that many relevant options? I assume implementing rotation should be sufficient for 99% of the test use cases. This feels simpler than reimplementing all functions that create text. You start with xlabel/ylabel, but remarked yourself, there are issues with suptitles. Then we may need ticks. Then it would be helpful to use this for legends etc.

But it's just a suggestion. I won't block over this.

@jklymak
Copy link
Member Author

jklymak commented Mar 31, 2025

I could try monkey patching, but it'd take me a while to sort out how to do it, and maybe not in time for #29827 which this was aimed at. Note that it is not trivial to place rectangles relative to an axes, but have a fixed physical size, and we place x and y labels by avoiding ticks so we would need to make sure the extents are correct for whatever is monkey patched. It may be worth the effort, but it's not trivial.

@timhoffm
Copy link
Member

timhoffm commented Apr 1, 2025

As a very minimal test

--- a/lib/matplotlib/text.py
+++ b/lib/matplotlib/text.py
@@ -796,6 +796,7 @@ class Text(Artist):
                 mtext = self if len(info) == 1 else None
                 x = x + posx
                 y = y + posy
+                Rectangle((x, y), wh[0], wh[1]).draw(renderer)
                 if renderer.flipy():
                     y = canvash - y
                 clean_line, ismath = self._preprocess_math(line)

marks the text positions well. Rotation must still be added, you'll want to replace the actual sizes with dummy sizes to not depend on the font, and you want to pull this out of the middle of draw. But it looks quite doable.

plt.plot([1, 3, 2])
plt.xlabel("foo")
plt.ylabel("test")
plt.title("title")
plt.show()

grafik

@tacaswell tacaswell added this to the v3.11.0 milestone Apr 3, 2025
@tacaswell
Copy link
Member

This is a good idea, but I do not think we want to block the freetype 2.13.3 adoption on it.

@jklymak
Copy link
Member Author

jklymak commented Apr 5, 2025

Closed in lieu of #29872

@jklymak jklymak closed this Apr 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants