Fix confusion between text height and ascent in metrics calculations.#31107
Fix confusion between text height and ascent in metrics calculations.#31107anntzer wants to merge 1 commit intomatplotlib:text-overhaulfrom
Conversation
lib/matplotlib/text.py
Outdated
| xys = M.transform(offset_layout) - (offsetx, offsety) | ||
|
|
||
| return bbox, list(zip(lines, zip(ws, hs), *xys.T)), descent | ||
| return bbox, list(zip(lines, wads, *xys.T)), descent |
There was a problem hiding this comment.
Do we still need to return descent now that we have d in wads?
There was a problem hiding this comment.
Good catch, it was anyways unused; removed.
|
You need to also update this call: matplotlib/lib/matplotlib/offsetbox.py Line 802 in d68c7e3
|
|
Oops, I missed that call site; fixed. |
|
Code looks good. Not checked, but I assume the image comparison failures are from slight text changes? What's the plan to handle them? |
|
I don't see anything obviously wrong in the code so far. This PR targets the There are 248 test image failures with this change, but they are a bit inconsistent in how they manifest. For example, in |
|
Agreed that it is hard to judge whether most changes (in non-multiline text) are really better or worse, except the patheffect legend (test_patheffect3) which appears clearly better (more vertically centered in the box) to me. old |
|
My read is that this makes the code match what the comments said it actually did. |
timhoffm
left a comment
There was a problem hiding this comment.
Looks good apart from the vauge naming.
The text height includes both the ascender and the descender, but the logic in _get_layout is that multiline texts should be treated as having an ascent at least as large as "l" and a descent at least as large as "p" (not a height at least as large as "lp" and a descent at least as large as "p") to prevent lines from bumping into each other (see changes to test_text/test_multiline, where the topmost superscript was close to bumping into the "p" descender previously).
The text height includes both the ascender and the descender, but the logic in _get_layout is that multiline texts should be treated as having an ascent at least as large as "l" and a descent at least as large as "p" (not a height at least as large as "lp" and a descent at least as large as "p") to prevent lines from bumping into each other (see changes to test_text/test_multiline, where the topmost superscript was close to bumping into the "p" descender previously).
Partial fix for #21653; see #21653 (comment).
old
new
PR summary
PR checklist