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

Align vinculum of fractions using minus sign instead of equals sign#20627

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 merge0 commits intomatplotlib:mainfromtfpf:mathtext-vertical-align

Conversation

tfpf
Copy link
Contributor

@tfpftfpf commentedJul 10, 2021
edited
Loading

PR Summary

This PR attempts to fix the vertical alignment of fractions. The horizontal line separating the numerator from the denominator should be at the same level as a minus sign (if it were present). Currently, the alignment uses the equals sign.

The fraction always appears slightly below where it should.
before_dejavusans

With this fix, the alignment is a little less shabby.
after_dejavusans

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant. (No errors from pyflake in the changed part.)
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • [N/A] Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • [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).

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 10, 2021
edited
Loading

The alignment may seem to be off by a pixel in some of the fractions, but I believe that is because of the limitations of the PNG format. If the images are saved using the SVG format, it is clear (upon zooming in) that the alignment is correct.

Here's the test code.

import matplotlib as mplimport matplotlib.pyplot as pltfor font in ['dejavusans', 'cm']:    mpl.rcParams['mathtext.fontset'] = font    fig, ax = plt.subplots()    ax.axis('off')    for size in range(10, 101, 10):        ax.text(0, size / 20, r'$a=-\dfrac{b}{c}=-\dfrac{c}{b}=-\dfrac{.}{.}=-\dfrac{W}{.}=-\dfrac{.}{W}$', size=size)    plt.savefig(f'sizes_{font}.png')

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

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.

Thanks@tfpf. The result indeed seems correct. Considering the unicode minus is the right thing to do. I'm not a font guy, and I don't understand the details behind the calculation. Do you know why there is thethickness * x subtraction, and whyx=1.5 is the correct value rather thanx=3? Or is this just an empirical observation?

On a side note, several reference images have changed, which seems expected (but I did not have a look at them). I think we'll have to regenerate them.

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 19, 2021
edited
Loading

@timhoffm I wasn't sure if I'd be able to describe it properly, so I attached an image. Here,vlist.shift_amount has been set to zero. As a result, the fraction sits on the same line the rest of the text does.

01_no_shift

shift is to be determined.

The first thing I noticed was that the space between the numerator and the vinculum is not the same as the space between the denominator and the vinculum.According to the source code (lines 2768 to 2770), both should be2 * thickness. Surprisingly, the rendered spaces are3 * thickness andthickness respectively! (This is true irrespective of the font used.)

With that in mind,shift can be deduced from the figure.

shift = cden.height + 1.5 * thickness - (metrics.ymax + metrics.ymin) / 2

@jklymak
Copy link
Member

Latex gives us:

mathtext2

which is substantially different than what we have currently, or with this change. Note that theb andc are aligned at their baseline. I think the fundamental problem with the minus sign is that the offsets are not being applied properly between the numerator and the denominator.

I'm not sure if half fixing this is worth breaking so many images?

@QuLogic
Copy link
Member

The true algorithm is in the TeX booklinked here. I tried afew things that didn't quite get all the way there, but maybe with this it would work?

@jklymakjklymak marked this pull request as draftJuly 20, 2021 16:50
@jklymak
Copy link
Member

Lets pop this to draft:@tfpf not sure how feasible it is for you to look into a more holistic approach here? If not, then it seems maybe we should get it right rather than whack-a-mole with different alignments?

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 20, 2021
edited
Loading

That makes sense. I'll see if I can come up with a better solution.

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 23, 2021
edited
Loading

Tried to study the algorithm LaTeX uses to draw fractions, and also made some observations independently. It looks like the numerator and denominator are shifted upwards and downwards respectively from thebaseline. The shift amounts are font parameters, stored infont_info (an array).

Once the numerators and denominators have been shifted, LaTeX checks whether the actual clearance between them and the line is less than the minimum clearance. If so, LaTeX is happy to push them up or down to honour said minimum clearance (even if it means that all numerators and all denominators won't be on the same baseline), as seen below.

\dfrac{x}{x^2}\dfrac{x_{Hy}}{x}
image

However, since Matplotlib packs the elements of a fraction into aVlist, I figured it might better to follow the same approach, but with spaces inserted wherever required.

This is what I got (red lines added by me using Paint).
fix_dejavusans_lines

The minimum clearance used here is half of what is mentioned in the LaTeX book. I couldn't figure out any alternative to the hard-coded multipliers (3/5 and4/3), so I just let them be (for now).

@jklymak
Copy link
Member

That surelooks better. Thanks for looking at this so carefully - very much appreciated, as this was a lot more work than a quick change at this point.

Lets see how the tests shake out, and then hopefully there aren't any edge cases that this is not working on.

@tfpf
Copy link
ContributorAuthor

Almost all image comparison tests involving fractions will fail, though. Won't they?

Also, in hindsight, I guessvlist could probably be created directly instead of repeatedly appending items tovlist_builder.

@jklymak
Copy link
Member

For sure it will break the image tests. I just meant that hopefully it breaks them correctly ;-)

@jklymak
Copy link
Member

@tfpf When you get the new images, please include them as part of the PR (usually by copying fromresult_images intomatplotlib/lib/tests/baseline_images)

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 27, 2021
edited
Loading

I skimmed through a few image miscompares. Forstix,stixsans,dejavusans anddejavuserif, the results aren't too bad. Butcm doesn't behave well. An egregious example:

mathtext_cm_19.pngmathtext_cm_19-expected.png
mathtext_cm_19mathtext_cm_19-expected

The alignment of\frac{5}{2} in the numerator appears to be wrong, and the reference image seems to have got it right. However, if I try to generate the same fraction with a minus sign immediately after it:

failures

it immediately becomes clear that the problem isn't the alignment of the fraction, but the alignment (or perhaps the rendering) of the plus sign. It's off by just one pixel, but looks like a big change because of the font size.

I think this will significantly affect the image comparison tests: even if we get the alignment right, it'll look wrong at small font sizes.

ETA:@jklymak Apologies, I didn't see your comment. What do you think about this? Should I include the images in the PR in spite of this rendering problem?

@jklymak
Copy link
Member

I'd be careful with 1-pixel changes. Matplotlib does funny things at pixel resolution like snapping lines to the nearest pixel. It looks possible to me that it is doing that for the fraction lines. Can you turn snapping off for those, at least as a test of the algorithm?

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 27, 2021
edited
Loading

Passingsnap=False orsnap=True (is that the correct way to disable snapping?) tofig.text results in exactly the same image (the third image in my previous comment). Is that a bad sign?

@jklymak
Copy link
Member

I'm not familiar enough with the math mode to know. Maybe it's just hard coded in?

@tfpf
Copy link
ContributorAuthor

Okay, I found something interesting.mpl.rcParams['figure.dpi'] defaults to 100. My screen is rated 96 DPI. If I save that image withdpi=96, everything is perfectly aligned!

failures

And this is despite using the Agg backend.

As for it being hardcoded, I am not sure, either. I haven't studied the source code apart from the_genfrac function.

@jklymak
Copy link
Member

I get yelled at for this, but I'm not too fussed about 1-pixel offsets - publication quality is 300 dpi or above. As long as it looks good at 200 dpi, I would not block on inconsistencies down at 100 dpi for such tiny fonts. Unless I pixel-peep I can't really see the differences in the above two examples, other than the "expected" looks cramped.

@tfpf
Copy link
ContributorAuthor

tfpf commentedJul 27, 2021
edited
Loading

All right, then.

I want to take a look at some more image miscompares. For the next commit, I will add the images along with code changes (if any).

@tfpftfpf marked this pull request as ready for reviewJuly 30, 2021 04:01
@tfpf
Copy link
ContributorAuthor

tfpf commentedAug 1, 2021

Looks like I accidentally committedall images which changed (1048 out of 1488) instead of just the ones flagged by pytest as mismatches (nearly 200). Should I undo the previous commit and add only the latter?

@timhoffm
Copy link
Member

Yes please.

@tfpftfpfforce-pushed themathtext-vertical-align branch fromc51e6e1 to2d070d0CompareAugust 1, 2021 09:14
@tfpf
Copy link
ContributorAuthor

tfpf commentedAug 4, 2021

@matplotlib/developers Could someone take a look at the changes and review them?

@jklymak
Copy link
Member

@tfpf they are still not passing the tests, so not yet reviewable....

@tfpftfpf marked this pull request as draftAugust 4, 2021 17:39
@tfpf
Copy link
ContributorAuthor

tfpf commentedAug 4, 2021

There are some SVG miscompares, but I can't reproduce them on my machine. Pytest skips the SVG comparison tests. I tried installing Matplotlib as mentioned in thecontributing guidelines. Even tried supplyingtests = True andbackend = SVG in setup.cfg during installation.

$ pytest lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-cm-3]========================================== test session starts ===========================================platform linux -- Python 3.8.10, pytest-6.2.4, py-1.10.0, pluggy-0.13.1rootdir: /home/tfpf/Documents/Code/matplotlib/matplotlib, configfile: pytest.inicollected 1 item                                                                                         lib/matplotlib/tests/test_mathtext.py s                                                            [100%]=========================================== 1 skipped in 0.66s ===========================================

Any suggestions on what I did wrong will be appreciated.

@QuLogic
Copy link
Member

As a completely new clone, it started tracking when you checked out the branch:

$ git checkout mathtext-vertical-alignUpdating files: 100% (438/438), done.Branch 'mathtext-vertical-align' set up to track remote branch 'mathtext-vertical-align' from 'origin'.Switched to a new branch 'mathtext-vertical-align'

So it's fine to push.

If you really want to hold on to your original commits, you can dogit branch old-mathtext-vertical-align origin/mathtext-vertical-align to save them from before your rebase.

@tfpftfpfforce-pushed themathtext-vertical-align branch 2 times, most recently from873b41d toe437df8CompareOctober 22, 2021 19:53
@tfpf
Copy link
ContributorAuthor

tfpf commentedOct 29, 2021
edited
Loading

I don't understand … there are Pytest errors from lines in _mathtext.py which have not been changed in this PR. Specifically, errors from Pyparsing. No idea what's going on. And while those tests failed, the others passed.

I remember that every test had passed before the rebase.

@jklymak
Copy link
Member

Thats correct - there is some pyparsing instability at the moment that is actively being worked on. OTOH because this relies on math text, we should be sure to rebase once all the pyparsing issues are sorted out.

@jklymak
Copy link
Member

@jkseppan@aitikgupta did either of you have time to check this out? There are alot of image changes, but they seem much more correct than previously?

Copy link
Contributor

@aitikguptaaitikgupta left a comment
edited
Loading

Choose a reason for hiding this comment

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

To skim through generated images, I'd suggest looking at PDF/SVG backend ones, Agg at low DPI pretty much messes things up:

mathtext_cm_28.pngmathtext_cm_28.pdf
mathtext_cm_28-Aggmathtext_cm_28-PDF

(look at the upper half of the fraction, i.e., x=6/8)

Pytest skips the SVG comparison tests

I faced a similar issue a while back when generating baselines, and an installation of inkspace fixed it (as@jklymak mentioned). I think SVG backend fails to surface an error when inkscape isn't installed (probably deserves another issue thread).

Thanks@tfpf for exploring, this does indeed look correct. 💯

jklymak and tfpf reacted with thumbs up emoji
@jklymak
Copy link
Member

@tfpf did you have the ability to get the svg baseline images created? If not, someone else could probably push to your PR

@tfpf
Copy link
ContributorAuthor

@jklymak Yes, I have added all images, including the SVGs. (Installing Inkscape worked, as you had suggested.)

I suppose this will have to be rebased again? I did it once after'Refix for pyparsing compat.' was merged, but some tests failed despite that. This time, the errors don't seem to be caused by Pyparsing, but I am not sure.

@jklymak
Copy link
Member

I'm not sure either - the error is math text related though, so a rebase couldn't hurt.

@tfpftfpfforce-pushed themathtext-vertical-align branch from5d7414f tob15a1c1CompareNovember 25, 2021 15:32
@tfpf
Copy link
ContributorAuthor

tfpf commentedNov 26, 2021
edited
Loading

Looks like that worked.

Should this PR be linked tothis issue Qulogic mentioned?

@tfpf
Copy link
ContributorAuthor

tfpf commentedJan 6, 2022

It says there's a review pending. Are there any updates on that?

@tfpf
Copy link
ContributorAuthor

Following the developments in the\genfrac issue yesterday, I think it might be prudent to include a fix in this PR itself, because of the large number of images to be changed. Props to oscargus, without whom, over 300 images might potentially have been changed in vain!

I will update this PR with the changes soon. Moving to draft until then.

timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

Replaced by#22852.

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

@timhoffmtimhoffmtimhoffm left review comments

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@aitikguptaaitikguptaaitikgupta approved these changes

@jkseppanjkseppanAwaiting requested review from jkseppan

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vertical positioning in mathtext fraction rendering could be improved
6 participants
@tfpf@jklymak@QuLogic@timhoffm@tacaswell@aitikgupta

[8]ページ先頭

©2009-2025 Movatter.jp