Repository navigation
Fix stale usetex text metrics when text.latex.preamble changes - #32473
himanchalkaushale wants to merge 3 commits into
Conversation
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
\usepackage{helvet} only redefines the sans-serif default (\sfdefault); with a serif family the text is typeset with \rmfamily and loading helvet does not change its font, so the widths never differed and the test could not detect stale metrics. Set font.family to sans-serif so the text is typeset with \sffamily and the preamble change provably switches the measured font.
lucaznch
left a comment
lucaznch
left a comment
There was a problem hiding this comment.
Thank you for the contribution. The PR currently has some failing CI checks that seem related and worth investigating.
|
|
||
| # The Helvetica metrics differ from the previously cached Computer Modern | ||
| # ones, i.e. the stale metrics are not reused ... | ||
| assert all(width != cm_width for width in widths) |
There was a problem hiding this comment.
This assertion is failing in all six Ubuntu test runs.
There was a problem hiding this comment.
Thanks for pointing this out. All ubuntu checks are now passed in 3fa0b90 commit, but AppVeyor hit the 60-minute limit at 98% with no test failure.
There was a problem hiding this comment.
The appveyor check is known to time out, so I wouldn't worry too much about that one.
In general, make sure all CI checks pass on your pull request. It's also best to run the relevant tests locally before pushing your changes rather than relying on CI alone. See the testing guide for more information.
With the checks passing now, the PR should be in a better position for a maintainer to review.
Replace unreliable helvet/sans-serif setup with times/serif which produces a
large, deterministic metrics change in all TeX Live environments.
The original test used \usepackage{helvet} with font.family=sans-serif, but
in Ubuntu CI the cmss vs Helvetica metrics for alphanumeric text were
nearly identical, causing false failures.
The new test uses the default serif family (Computer Modern Roman) and
\usepackage{times} (mathptmx), which is the standard way to change the
serif font in LaTeX and produces a dramatic, reliable width difference.
Verifies cache invalidation by checking:
1. existing_width != cm_width (preamble change affects rendering)
2. existing_width == same_fig_width == new_fig_width (all three measurements
agree, proving stale cache is not reused)
Closes matplotlib#32413.
This branch has not been deployed
2 participants
Closes #32413
Summary
Fixes stale text metrics in
usetexmode whentext.latex.preamblechanges._get_text_metrics_with_cachecaches the results ofrenderer.get_text_width_height_descentusing the text, font properties, math mode, and DPI as cache keys. However, inusetexmode, the rendered dimensions also depend ontext.latex.preamble, which was not included in the cache key.As a result, changing the preamble could leave existing
Textobjects with stale metrics. Since Matplotlib 3.10 moved the cache from individualTextobjects to the renderer, newly createdTextobjects in the same figure could also reuse stale metrics. Creating a new figure avoided the issue because it created a new renderer with a fresh cache.This PR addresses the issue by including the LaTeX preamble in the cache key when
ismath == "TeX". For non-TeX text, the preamble remainsNone, preserving the existing cache-key behavior.The change is intentionally limited to the preamble. Other
TexManagerconfiguration options, such as font settings, are left unchanged because they are outside the scope of this issue.Tests
Adds a regression test that:
usetexenabled.\usepackage{helvet}.Textobject, a newly createdTextobject in the same figure, and aTextobject in a new figure all report consistent text widths.The test is skipped when the required
helvetLaTeX package is unavailable.The existing
test_metrics_cacheandtest_metrics_cache2assertions continue to pass.AI Disclosure
AI assistance was used to draft the regression test cases. The implementation and final test changes were reviewed by me.
PR Checklist