Fix confusion between text height and ascent in metrics calculations.#31107
Fix confusion between text height and ascent in metrics calculations.#31107QuLogic merged 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.
|
I've pushed the image changes to the |
|
Test failures look like the |
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).
Fix confusion between text height and ascent in metrics calculations.
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