-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix baseline alignment when using usetex. #16476
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
Conversation
698df42
to
497738c
Compare
def test_usetex(): | ||
mpl.rcParams['text.usetex'] = True | ||
fig = plt.figure() | ||
ax = fig.add_subplot(111) | ||
ax.text(0.1, 0.2, | ||
kwargs = {"verticalalignment": "baseline", "size": 24, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is significantly better 👍
(I'll combine this with #16410 as a single PR for the usual baseline image changes reason.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to ignore my comments, but this is full of so much state....
# push <= here, v is the baseline position. | ||
# etc. | ||
# (dviasm is useful to explore this structure.) | ||
self._baseline_v = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to put this in __init__
as well? Are we sure that _output
will never be called without _read
being called first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd already have errored out earlier anyways, because self.text wouldn't have been defined either.
b1c5b53
to
a494553
Compare
@tacaswell can you repro the failure locally? |
I can reproduce locally, though it's a slightly different RMS. |
Can you try regen'ing the baseline image locally and force-push? probably something about the version of freetype that dvipng is using... |
Problem is, now it's also doing:
|
Strange, I can't repro that failure locally. Tried rebasing on master, and also deleting ~/.cache/mpl/test_cache, still passing... |
I guess an additional diff --git i/lib/matplotlib/dviread.py w/lib/matplotlib/dviread.py
index bb14480e4..dcaba415a 100644
--- i/lib/matplotlib/dviread.py
+++ w/lib/matplotlib/dviread.py
@@ -278,6 +278,9 @@ class Dvi:
maxy_pure = self._baseline_v # This should normally be the case.
self._baseline_v = None
+ if not self.text and not self.boxes: # Avoid infs/nans from inf+/-inf.
+ return Page(text=[], boxes=[], width=0, height=0, descent=0)
+
if self.dpi is None:
# special case for ease of debugging: output raw dvi coordinates
return Page(text=self.text, boxes=self.boxes, may help, but I still don't know why I don't have an issue locally. numpy version, perhaps? |
My environment has lowest NumPy supported, 1.15.0. |
Does the proposed patch help? |
a494553
to
ce91174
Compare
Yes, it works. |
Looks like this still failed on the image diff. Do you know if we can download the test artefacts from e.g. azure to check the image and possibly just dump it in the baselines directory? |
But then it would just fail for everyone else off CI? #17011 for Azure artifacts, if it works. |
At least I want to know if it's just a freetype version problem (in which case we can just bump the tolerance) or something more serious. |
ce91174
to
7e09018
Compare
Looks like freetype-version related, will work on that... |
7e09018
to
d4aeacc
Compare
Looks like forcing freetype-less dvipng mode fixes the issue. It's a bit annoying to expose testing bits in texmanager, but I'm not really sure I want to expose that more because I have a further plan to just get rid of the dependency on dvipng altogether (for raster output we can actually just use the parsed dvi (as we already do for vector output) to extract the glyphs and draw them ourselves using our own freetype renderer -- that's what's done in mplcairo for example). This will skip a subprocess call (always good on Windows in particular), and of the FreeType version problem (we don't control the FreeType version used by dvipng, and FreeType-less mode is broken in dvipng 1.16 -- fixed in the most recent 1.17 and was good before 1.16 too, but version bounds on dvipng for tests only may be a bit painful...). Or we can first wait for that feature to go in... |
d4aeacc
to
ccabb61
Compare
Unfortunately at least some dvipngs appear to be statically linked to FreeType so we can't play tricks with LD_PRELOAD/LD_LIBRARY_PATH.
Currently, `figtext(.5, .5, "%foo", usetex=True)` results in an obscure exception (`FileNotFoundError: No such file or directory: blah.dvi`). Fix that by forcing tex to always generate a page, by inserting at least an empty \hbox{}.
Previously we inferred the baseline of usetex strings with the baseline of the character with the lowest baseline, which is wrong in the presence of indices (see usetex_baseline_test.py). There was an option to use the "preview" latex package to fix that, but that was never made the default and likely cannot as the package is GPL. Instead I can infer the baseline position by "reverse-engineering" the starting instructions of the dvi stream (this was done without consulting the approach in "preview"). The results can be checked using usetex_baseline_test.py (preview=False now looks the same as preview=True; likewise, the output of test_usetex was changed. The text.latex.preview rc can now be deprecated (in a future PR). The test was also updated to include the strings of the usetex_baseline_test example (which may go away once text.latex.preview is removed).
ccabb61
to
8472fbb
Compare
Fix baseline alignment when using usetex. Conflicts: lib/matplotlib/tests/test_usetex.py - import np and formatting issues on decorator
Previously we inferred the baseline of usetex strings with the baseline
of the character with the lowest baseline, which is wrong in the
presence of indices (see usetex_baseline_test.py). There was an option
to use the "preview" latex package to fix that, but that was never made
the default and likely cannot as the package is GPL.
Instead I can infer the baseline position by "reverse-engineering"
the starting instructions of the dvi stream (this was done without
consulting the approach in "preview"). The results can be checked
using usetex_baseline_test.py (preview=False now looks the same as
preview=True; likewise, the output of test_usetex was changed.
(see #16373)
The text.latex.preview rc can now be deprecated (in a future PR).
The test was also updated to include the strings of the
usetex_baseline_test example (which may go away once text.latex.preview
is removed). (The baseline image was already changing, in any case...)
I think the test is "spuriously" failing because my local version of latex tools are not compiled against the same freetype version as on CI :/ Basically the baseline image will need to be regen'd by someone who can otherwise already pass test_usetex.py locally.
Edit: Now also includes #16410 as first commit (that PR has been closed), so closes #16409.
... but still can't repro the failure locally.
PR Summary
PR Checklist