GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||
GeezerVison is showing github.com — Fix stale usetex text metrics when text.latex.preamble changes by himanchalkaushale · Pull Request #32473 · matplotlib/matplotlib ConversationCloses #32413 SummaryFixes stale text metrics in
As a result, changing the preamble could leave existing This PR addresses the issue by including the LaTeX preamble in the cache key when The change is intentionally limited to the preamble. Other TestsAdds a regression test that:
The test is skipped when the required The existing AI DisclosureAI assistance was used to draft the regression test cases. The implementation and final test changes were reviewed by me. PR Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. \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
reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the contribution. The PR currently has some failing CI checks that seem related and worth investigating.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion is failing in all six Ubuntu test runs.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsReplace 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.
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. This branch has not been deployedNo deployments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
ReviewersAt least 1 approving review is required to merge this pull request. Uh oh!
There was an error while loading. Please reload this page. AssigneesNo one assignedLabelsProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. [Bug]: Changing text.latex.preamble does not invalidate cached usetex text metricsUh oh!
There was an error while loading. Please reload this page. Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later. | ||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |