| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
A few notes. AlgorithmTo best of my knowledge, I tried to use the algorithm described in TeX: The Program by Don Knuth. It describes numerators and denominators as being shifted independently of each other, whereas Matplotlib packs them (along with a rule vertical spaces) into a box, and moves the box down. As a result, some acrobatics were necessary, so there is scope of optimisation, but I focused on getting it to work first.
And yes, the spacing does take into account the thickness of the rule, as TeX does (fix #22172). Hopefully, it will also fix #18389. Font ConstantsIn order to use the same algorithm, I defined five new constants in the class FontConstantsBase, and used them to mean the same thing TeX does. (On an unrelated note, the comments describing these constants refer to them as 'percentages', but I think they should be called 'multipliers', since they are direct multipliers. Should the comments be changed?) I chose values which best rendered fractions while using DejaVu Sans. Different values may be needed for the other four typefaces. Possible Bug in Fraction Rendering num_clr = max(
num_shift_up - cnum.depth - axis_height - rule / 2, min_clr)
den_clr = max(
axis_height - rule / 2 + den_shift_down - cden.height, min_clr)
vlist = Vlist([cnum, # numerator
Vbox(0, num_clr - rule), # space
Hrule(state, rule), # rule
Vbox(0, den_clr + rule), # space
cden # denominator
])
This is the block of code which defines the clearance between the rule and the numerator/denominator. num_clr is the vertical distance between the numerator and the rule. den_clr is that between the denominator and the rule. If a Vlist is created with num_clr and den_clr, the actual rendered distances turn out to be num_clr + ruleand den_clr - rule. This looks like a bug to me, so I undid its effects by specifying num_clr - rule and den_clr + rule. The actual rendered distances then become num_clr - rule + rule = num_clr and den_clr + rule - rule = den_clr, which matches what TeX would do. If the bug gets fixed in the future, all we'd need to do is remove - rule and + rule from the above code block. Updating ImagesThe choice of font constants may work for DejaVu Sans, but probably won't work for the others. I think images should be added only after we have a consensus on what those values should be. |
Sorry, something went wrong.
There was a problem hiding this comment.
If you have it, including a reference to the TeX algorithm would be great.
Sorry, something went wrong.
Sure. I'll add a comment with the appropriate references. |
Sorry, something went wrong.
|
I have added font constants for STIX, STIX sans, DejaVu Serif and CM. The values chosen are the nearest integral multiples of 0.1 for which the numerator and denominator seen in the images in this comment do not have to be shifted beyond their default shift amounts. If that sounds okay, I will try to update the images as well. |
Sorry, something went wrong.
|
Sorry to hijack this PR (which looks nice, although I haven't checked the exact algorithms yet), but perhaps we can revisit #19201 (smaller svg baselines by not embedding fonts) as well? (at least the part switching the svg baselines to not embedding the fonts, for a significant gain in size, even if we keep the png and pdf baselines still around for now.) #19201 was deferred on the expectation of switching to a completely separate baseline images repo, but I'm not sure this is going to happen soon... |
Sorry, something went wrong.
|
That is a good idea (as I understand, fewer changes to the images will result in a smaller size increase of the repo), but I am really not sure how to do it. Perhaps someone else could push the image change commits here? |
Sorry, something went wrong.
Sorry, something went wrong.
|
Agreed it looks better without the shift; a quick git blame suggests this comes from the original implementation (c10b4bb) with no reason given :-( |
Sorry, something went wrong.
Sorry, something went wrong.
That is unfortunate.😐 At a glance, it appears that this space was added so that the lines of two consecutive fractions (e.g. \dfrac{1}{2}\dfrac{1}{2}) do not look like a single line. But that raises the question why the space was not added on both sides like this: result = [Hlist([Hbox(thickness * 1.), vlist, Hbox(thickness * 1.)])] instead of just on the right. Although it's just a single change, I think this deserves a separate PR? |
Sorry, something went wrong.
|
Seems like a reasonable guess as to why this came in. Do you have any idea of what TeX does to avoid the same problem? |
Sorry, something went wrong.
|
From a quick look at the book, node 748 seems to suggest that the Hlist itself does not contain any extra space. Perhaps a space is added only if there is something before or after the fraction? I have no clue. |
Sorry, something went wrong.
I think I've got it. Per node 764 of the book, all possible inter-noad space codes are first stored in a 64-element string, which is treated as if it were an 8-by-8 matrix. The space between two noad types a and b is computed using the element in the ath row and bth column of the matrix, along with what is called a magic_constant. (I think that's the general idea.) Here, a and b are numbers representing the noad type. A noad is an element of an mlist (which is a linked list representing formulae). Hence, the space between two fractions is the 8 * fraction_noad + fraction_noad + magic_offsetth element of the matrix. Is this beyond the scope of Mathtext? (Will there be any side-effects of making this change.) |
Sorry, something went wrong.
|
Moving to draft. Feel free to move back when the relevant tests pass! |
Sorry, something went wrong.
|
@tfpf I think best would be to perhaps not encode the entire 64 internoad spaces for now (unless you want to go for it) and just specifically encode an inter-fraction noad? Then perhaps squash this together with #21850 and accept the mass changes in baseline images (although I would much prefer if #22881 could go in first, too, and switch all the changed images to svg-unembedded-glyphs-only). |
Sorry, something went wrong.
As far as I can tell, Mathtext's spacing algorithm is not the same as that of TeX. Mathtext adds spaces to Hlist objects. I think it might be simpler to just put equal spaces on both sides of the fraction.
Agreed, that is the most meaningful thing to do. I will hold off on changing any images until that pull request is merged. |
Sorry, something went wrong.
@anntzer Does this mean: add the subscript and superscript changes here? This would result in one less change for some images, because there are some tests which have fractions as well as superscripts and subscripts. However, I had to cut some corners for the latter. I'll paste the details here.
Also, if we are to use the TeX algorithm for superscripts and subscripts, the "poor man's x-height" calculation (#21850 (comment)) may have to be forced for all fonts. (If there is a better way, I am all for it, though.) |
Sorry, something went wrong.
Yes.
IIRC(?) there are other places where we noted that font-reported-x-heights can be useless and it may be better to use poor man's x-height everywhere instead, so I wouldn't be too annoyed by that change. The shortcuts you mention seem fine. |
Sorry, something went wrong.
|
Of the 537 Mathtext image comparison tests failed, there were some that failed for Computer Modern, but not for the other fonts. I moved all the failing tests to svgastext_math_tests, but since each test checks all five fonts, a total of 720 new images have to added.:sweat_smile: There are a few errors I don't quite understand.
Also, I can't quite figure out how to identify the old images to be deleted. |
Sorry, something went wrong.
This would mean that some images can simply be renamed instead of being deleted and added back … if one could sift through all of them. All-in-all, this is a probably too big a change, so I'll be happy to close it if it doesn't work out. |
Sorry, something went wrong.
|
Sorry, I slightly messed up the "svg lightweight tests" change. Can you additionally include the two lines at https://github.com/matplotlib/matplotlib/pull/23270/files#diff-17f9692b77108a62b59c71825d8461488729ef280f56785d3ad55ce3a86044d8R210 (as that doesn't look like it's getting merged any time soon) to your PR? This way only svg will be tested, and only for dejavusans and cm; you can then remove the other baselines. I don't think(?) there's a really smart way to figure out what images can be deleted other than by manually checking... (Probably it would be nice to have some tooling for that.) |
Sorry, something went wrong.
|
Sure, I will update it. |
Sorry, something went wrong.
|
PR cleanliness failed. If these changes are acceptable, I will squash the commits. |
Sorry, something went wrong.
|
Previously, the two tests r'$xyz^kx_kx^py^{p-2} d_i^jb_jc_kd x^j_i E^0 E^0_u$', # github issue #4873
r'${xyz}^k{x}_{k}{x}^{p}{y}^{p-2} {d}_{i}^{j}{b}_{j}{c}_{k}{d} {x}^{j}_{i}{E}^{0}{E}^0_u$',rendered the same. With this PR, some of the exponents are shifted between the two cases. I think this is not desirable? |
Sorry, something went wrong.
|
Probably a result of the difference between {something} and something, as discussed in #23257. I'll try to figure it out. |
Sorry, something went wrong.
|
This time, I double-checked that x^2_y and x_y^2 are rendered identically, as are xyz^kx_kx^py^{p-2} d_i^jb_jc_kd x^j_i E^0 E^0_u and {xyz}^k{x}_{k}{x}^{p}{y}^{p-2} {d}_{i}^{j}{b}_{j}{c}_{k}{d} {x}^{j}_{i}{E}^{0}{E}^0_u. Another thing is that {a}_{0}+\dfrac{1}{{a}_{1}+\dfrac{1}{{a}_{2}+\dfrac{1}{{a}_{3}+\dfrac{1}{{a}_{4}}}}} gets cut off at the top and bottom (the size of the figure isn't enough accommodate everything). |
Sorry, something went wrong.
As described in *TeX: the Program* by Don Knuth. New font constants are set to the nearest integral multiples of 0.1 for which numerators and denominators containing normal text do not have to be shifted beyond their default shift amounts at font size 30 in display and text styles. To better process superscripts and subscripts, the x-height is now always calculated instead of being retrieved from the font table (which was the case for Computer Modern); the affected font constants have been changed. Mathtext tests which failed as a result of these changes have been moved from `math_tests` to `svgastext_math_tests`. A duplicate test was also fixed in the process.
Sorry, something went wrong.
Sorry, something went wrong.
|
The fact that the svg cm misrenders on a webbrowser is normal: this is because it is specifically tailored to use matplotlib's cm font files, which are the "classical" ones from AMS and use a nonstandard encoding: unless you specifically point the the right font files (which our _SVGConverter does), you won't get the right glyphs. So the idea is really to just look at the dejavu svgs; if you want to confirm that the cm svgs are correct you should run the tests and visualize the files converted to png in result_images/. |
Sorry, something went wrong.
I am still unable to figure this out, after all this time. Here's an example which demonstrates the problem. I made the following changes: diff --git a/lib/matplotlib/_mathtext.py b/lib/matplotlib/_mathtext.py
index 3a934c21fd..93df6c4f05 100644
--- a/lib/matplotlib/_mathtext.py
+++ b/lib/matplotlib/_mathtext.py
@@ -334,6 +334,10 @@ class TruetypeFonts(Fonts):
font = self._get_font(fontname)
font.set_size(fontsize, dpi)
pclt = font.get_sfnt_table('pclt')
+ print(f'{fontname=}, {fontsize=}', end=', ')
+ if pclt is not None:
+ print('Reported:', (pclt['xHeight'] / 64.0) * (fontsize / 12.0) * (dpi / 100.0), end=', ')
+ print('Actual:', self.get_metrics(fontname, mpl.rcParams['mathtext.default'], 'x', fontsize, dpi).iceberg)
if pclt is None:
# Some fonts don't store the xHeight, so we do a poor man's xHeight
metrics = self.get_metrics(and created the file test.py. import matplotlib.pyplot as plt
plt.rcdefaults()
plt.rc('mathtext', fontset='cm')
plt.rc('font', size=12)
plt.figure().text(.5, .5, r'$x_1^2$')
plt.show()
Now comes the strange part. $ python3 test.py fontname='it', fontsize=12.0, Reported: 14.171875, Actual: 8.0 $ pytest -s lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-cm-12] ============================= test session starts ============================== platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0 rootdir: /home/tfpf/Documents/projects/matplotlib, configfile: pytest.ini plugins: xdist-2.5.0, forked-1.4.0 collected 1 item lib/matplotlib/tests/test_mathtext.py fontname='it', fontsize=12.0, Reported: 14.171875, Actual: 7.375 fontname='it', fontsize=12.0, Reported: 14.171875, Actual: 7.375 . ============================== 1 passed in 0.77s =============================== The x-height changes, presumably because the hinting is different in the tests. (Also, the ratio of the reported x-height height to the actual x-height of CM is not constant across font sizes.) I'll shelve this for a time I am better equipped to fix it. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Follow-up to #20627, which somehow got closed automatically when I was messing around with merge conflicts in my fork. I have implemented the same algorithm TeX uses to align fractions.
At the moment, I have not updated the images, for reasons I will mention in a comment.
Fixes #18086. Fixes #18389. Fixes #22172.
PR Checklist
Tests and Styling
Documentation