Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Implement TeX's fraction alignment for Mathtext fractions#22852

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
tfpf wants to merge1 commit intomatplotlib:mainfromtfpf:mathtext-vertical-align

Conversation

tfpf
Copy link
Contributor

@tfpftfpf commentedApr 15, 2022
edited
Loading

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

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@tfpf
Copy link
ContributorAuthor

tfpf commentedApr 15, 2022
edited
Loading

A few notes.

Algorithm

To best of my knowledge, I tried to use the algorithm described inTeX: 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.

fractions-with-without-rule
Fractions with and without rules: text style on the left, and display style on the right.

And yes, the spacing does take into account the thickness of the rule, as TeX does (fix#22172).
image

Hopefully, it will also fix#18389.

Font Constants

In order to use the same algorithm, I defined five new constants in the classFontConstantsBase, 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 aVlist is created withnum_clr andden_clr, the actual rendered distances turn out to benum_clr + ruleandden_clr - rule. This looks like a bug to me, so I undid its effects by specifyingnum_clr - rule andden_clr + rule. The actual rendered distances then becomenum_clr - rule + rule = num_clr andden_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 Images

The 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.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you have it, including a reference to the TeX algorithm would be great.

@tfpf
Copy link
ContributorAuthor

If you have it, including a reference to the TeX algorithm would be great.

Sure. I'll add a comment with the appropriate references.

@tfpf
Copy link
ContributorAuthor

tfpf commentedApr 15, 2022
edited
Loading

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 inthe 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.

@anntzer
Copy link
Contributor

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...
(I can extract the relevant parts of#19201 into a new PR, of course.)

@tfpf
Copy link
ContributorAuthor

tfpf commentedApr 20, 2022
edited
Loading

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?

timhoffm reacted with thumbs up emoji

@tfpftfpf changed the titleImplement TeX's fraction alignmentImplement TeX's fraction alignment for Mathtext fractionsApr 20, 2022
@tfpf
Copy link
ContributorAuthor

tfpf commentedApr 21, 2022
edited
Loading

Noticed something while fixing the merge conflict. Mathtext adds a space to the right of each fraction.

    result = [Hlist([vlist, Hbox(thickness * 2.)])]

Is there a reason for this? Fractions look better without that space:
without_space

than with it.
with_space

Brackets added to make the difference clear.

@anntzer
Copy link
Contributor

Agreed it looks better without the shift; a quick git blame suggests this comes from the original implementation (c10b4bb) with no reason given :-(

@anntzer
Copy link
Contributor

I put up a revival of#19201 at#22881, let's discuss that there.

@tfpf
Copy link
ContributorAuthor

from the original implementation with no reason given

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?

@anntzer
Copy link
Contributor

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?

@tfpf
Copy link
ContributorAuthor

From a quick look at the book, node 748 seems to suggest that theHlist 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.

@tfpf
Copy link
ContributorAuthor

tfpf commentedMay 2, 2022
edited
Loading

Do you have any idea of what TeX does to avoid the same problem?

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 typesa andb is computed using the element in theath row andbth column of the matrix, along with what is called amagic_constant. (I think that's the general idea.)

Here,a andb 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 the8 * fraction_noad + fraction_noad + magic_offsetth element of the matrix.

Is this beyond the scope of Mathtext? (Will there be any side-effects of makingthis change.)

@jklymak
Copy link
Member

Moving to draft. Feel free to move back when the relevant tests pass!

@jklymakjklymak marked this pull request as draftMay 16, 2022 08:31
@anntzer
Copy link
Contributor

@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 wouldmuch prefer if#22881 could go in first, too, and switch all the changed images to svg-unembedded-glyphs-only).

@tfpf
Copy link
ContributorAuthor

just specifically encode an inter-fraction noad

As far as I can tell, Mathtext's spacing algorithm is not the same as that of TeX. Mathtext adds spaces toHlist objects. I think it might be simpler to just put equal spaces on both sides of the fraction.

switch all the changed images to svg-unembedded-glyphs-only

Agreed, that is the most meaningful thing to do. I will hold off on changing any images until that pull request is merged.

@tfpf
Copy link
ContributorAuthor

tfpf commentedJun 13, 2022
edited
Loading

Then perhaps squash this together

@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.

Shortcuts I took for the algorithm

  • The default values ofshift_up andshift_down depend on two font constants,supdrop andsubdrop, respectively. However, Mathtext usessubdrop for both (as the comment in theFontConstantsBase class mentions), so I did the same.
  • The actual value ofshift_up depends on the current style (display, text, script or script-script), and may be calculated usingsup1,sup2 orsup3. However, superscripts and subscripts in Mathtext do not know about the current style (Mathtext only has thesup1 constant); they are simply one size smaller than the nucleus. I've also used onlysup1.

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.)

@anntzer
Copy link
Contributor

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.

Yes.

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.)

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.

@tfpftfpfforce-pushed themathtext-vertical-align branch 2 times, most recently from0e7bf54 tob65d493CompareJune 16, 2022 13:24
@tfpf
Copy link
ContributorAuthor

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 tosvgastext_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.

  • r'$\sqrt{1+\sqrt{1+\sqrt{1+\sqrt{1+\sqrt{1+\sqrt{1+\sqrt{1+x}}}}}}}$' (There are neither fractions nor superscripts or superscripts. And yet, this image failed the image comparison test.)
  • r"$f'\quad f'''(x)\quad ''/\mathrm{yr}$" (Looks like apostrophes affected by the superscript positioning.)
  • test_operator_space (In thischeck_figures_equal test, the 's' of 'cos' is displaced slightly compared to the reference image, even though 'co' is placed identically. This is strange, because operator kerning should have remained unchanged.)

Also, I can't quite figure out how to identify the old images to be deleted.

@tfpf
Copy link
ContributorAuthor

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:

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.

@anntzer
Copy link
Contributor

anntzer commentedJun 20, 2022
edited
Loading

Sorry, I slightly messed up the "svg lightweight tests" change. Can you additionally include the two lines athttps://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.)

@tfpf
Copy link
ContributorAuthor

Sure, I will update it.

@tfpf
Copy link
ContributorAuthor

Thanks a lot for the patient and detailed analysis! I think I miscalculated the font constant updates for CM. Further, I hadn't changeddelta to account for the changed x-height (as indicated by the kerning before '2'). If I fix those two things, maybe it'll work.

Also, note that the kerns before the "c" are actually different in the two supposedly identical examples (is that a bug?)

Definitely looks like one. Might be worth a closer look.

@tfpf
Copy link
ContributorAuthor

Also, note that the kerns before the "c" are actually different in the two supposedly identical examples (is that a bug?)

The space before 'c' is\,, which is 1/6 em. Looking at the definition ofTruetypeFonts._get_info, it looks likek2.44 andk2.31 are both 1/6 em, but the former uses an italicised 'm', whereas the latter uses a regular 'm'.

the kern before the "2" changed. (Why?)

This can be fixed by increasing the CM constants by the correct multiplying factor, but the problem is that the multiplying factor depends on the ratio of the old x-height and the actual x-height (as returned byget_metrics). The actual x-height, in turn, depends on the hinting type. The functionTruetypeFonts._get_info loads the metrics using the hinting type. I am not clear on what that exactly is, but if the hinting type is 2, the x-height of CM at font size 12 with 100 DPI is 7.375, whereas if the hinting type is 32, the x-height is 8 at the same font size and DPI.

I will go ahead and use the latter to calculate the multiplying factor, since that fixestest_operator_space and uses the same kerning as earlier.

@tfpftfpfforce-pushed themathtext-vertical-align branch from702e794 to3f6ed6cCompareJune 24, 2022 12:31
@tfpftfpfforce-pushed themathtext-vertical-align branch from22faffc toe3b1ec4CompareJuly 10, 2022 08:39
@tfpftfpf marked this pull request as ready for reviewJuly 10, 2022 12:47
@tfpf
Copy link
ContributorAuthor

PR cleanliness failed.

If these changes are acceptable, I will squash the commits.

@anntzer
Copy link
Contributor

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 #4873r'${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?

@tfpftfpf marked this pull request as draftJuly 11, 2022 04:51
@tfpf
Copy link
ContributorAuthor

Probably a result of the difference between{something} andsomething, as discussed in#23257. I'll try to figure it out.

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 15, 2022
edited
Loading

This time, I double-checked thatx^2_y andx_y^2 are rendered identically, as arexyz^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).

image
image

@tfpftfpf marked this pull request as ready for reviewJuly 15, 2022 15:54
@tfpftfpfforce-pushed themathtext-vertical-align branch from2693862 to02dff6aCompareJuly 17, 2022 08:28
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.
@QuLogic
Copy link
Member

There are alot of images changed here, but because they're renamed, it's difficult to really see what effect this change has on any of them. And the ones that are left with a diff sometimes look worse...

Plus some of the added images just don't look right at all:
image
(presumably due to missing fonts locally?)

If we want to combine this change with switch to SVG text, then we can do thatafter review, or just in an entirely separate commit/PR.

@tfpftfpf marked this pull request as draftAugust 10, 2022 05:06
@tfpf
Copy link
ContributorAuthor

tfpf commentedAug 10, 2022
edited
Loading

(presumably due to missing fonts locally?)

This is probably the reason, since Inkscape is able to correctly convert the SVG images to PNG. For instance, this ismathtext0_cm_07_svg.png.
image

If we want to combine this change with switch to SVG text, then we can do that after review, or just in an entirely separate commit/PR.

I guess moving to SVG in a separate PR will be easier to manage.

@anntzer
Copy link
Contributor

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/.

@tfpf
Copy link
ContributorAuthor

tfpf commentedNov 23, 2022
edited
Loading

Also, note that the kerns before the "c" are actually different in the two supposedly identical examples (is that a bug?)

The space before 'c' is ,, which is 1/6 em. Looking at the definition of TruetypeFonts._get_info, it looks like k2.44 and k2.31 are both 1/6 em, but the former uses an italicised 'm', whereas the latter uses a regular 'm'.

the kern before the "2" changed. (Why?)

This can be fixed by increasing the CM constants by the correct multiplying factor, but the problem is that the multiplying factor depends on the ratio of the old x-height and the actual x-height (as returned by get_metrics). The actual x-height, in turn, depends on the hinting type. The function TruetypeFonts._get_info loads the metrics using the hinting type. I am not clear on what that exactly is, but if the hinting type is 2, the x-height of CM at font size 12 with 100 DPI is 7.375, whereas if the hinting type is 32, the x-height is 8 at the same font size and DPI.

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.pyindex 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 filetest.py.

import matplotlib.pyplot as pltplt.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.0rootdir: /home/tfpf/Documents/projects/matplotlib, configfile: pytest.iniplugins: xdist-2.5.0, forked-1.4.0collected 1 item                                                               lib/matplotlib/tests/test_mathtext.py fontname='it', fontsize=12.0, Reported: 14.171875, Actual: 7.375fontname='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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@timhoffmtimhoffmtimhoffm left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
5 participants
@tfpf@anntzer@jklymak@QuLogic@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp