| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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 mpl
import matplotlib.pyplot as plt
for 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')
|
Sorry, something went wrong.
There was a problem hiding this comment.
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 us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
There was a problem hiding this comment.
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 the thickness * x subtraction, and why x=1.5 is the correct value rather than x=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.
Sorry, something went wrong.
|
@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. 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 be 2 * thickness. Surprisingly, the rendered spaces are 3 * thickness and thickness 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 |
Sorry, something went wrong.
|
Latex gives us: which is substantially different than what we have currently, or with this change. Note that the b and c 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? |
Sorry, something went wrong.
|
The true algorithm is in the TeX book linked here. I tried a few things that didn't quite get all the way there, but maybe with this it would work? |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
That makes sense. I'll see if I can come up with a better solution. |
Sorry, something went wrong.
Sorry, something went wrong.
|
That sure looks 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. |
Sorry, something went wrong.
|
Almost all image comparison tests involving fractions will fail, though. Won't they? Also, in hindsight, I guess vlist could probably be created directly instead of repeatedly appending items to vlist_builder. |
Sorry, something went wrong.
|
For sure it will break the image tests. I just meant that hopefully it breaks them correctly ;-) |
Sorry, something went wrong.
|
@tfpf When you get the new images, please include them as part of the PR (usually by copying from result_images into matplotlib/lib/tests/baseline_images) |
Sorry, something went wrong.
|
I skimmed through a few image miscompares. For stix, stixsans, dejavusans and dejavuserif, the results aren't too bad. But cm doesn't behave well. An egregious example:
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: 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? |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
Passing snap=False or snap=True (is that the correct way to disable snapping?) to fig.text results in exactly the same image (the third image in my previous comment). Is that a bad sign? |
Sorry, something went wrong.
|
I'm not familiar enough with the math mode to know. Maybe it's just hard coded in? |
Sorry, something went wrong.
|
Okay, I found something interesting. mpl.rcParams['figure.dpi'] defaults to 100. My screen is rated 96 DPI. If I save that image with dpi=96, everything is perfectly aligned! 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
Looks like I accidentally committed all 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? |
Sorry, something went wrong.
|
Yes please. |
Sorry, something went wrong.
|
@matplotlib/developers Could someone take a look at the changes and review them? |
Sorry, something went wrong.
|
@tfpf they are still not passing the tests, so not yet reviewable.... |
Sorry, something went wrong.
|
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 the contributing guidelines. Even tried supplying tests = True and backend = 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.1 rootdir: /home/tfpf/Documents/Code/matplotlib/matplotlib, configfile: pytest.ini collected 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@jkseppan @aitikgupta did either of you have time to check this out? There are a lot of image changes, but they seem much more correct than previously? |
Sorry, something went wrong.
There was a problem hiding this comment.
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.png | mathtext_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. 💯
Sorry, something went wrong.
|
@tfpf did you have the ability to get the svg baseline images created? If not, someone else could probably push to your PR |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
I'm not sure either - the error is math text related though, so a rebase couldn't hurt. |
Sorry, something went wrong.
|
Looks like that worked. Should this PR be linked to this issue Qulogic mentioned? |
Sorry, something went wrong.
|
It says there's a review pending. Are there any updates on that? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.

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

PR Checklist