Skip to content
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

[Bug]: Incorrect mathtext rendering of r"$\|$" with default (dejavu) math fontfamily #23250

Closed
anntzer opened this issue Jun 11, 2022 · 13 comments · Fixed by #24239
Closed

[Bug]: Incorrect mathtext rendering of r"$\|$" with default (dejavu) math fontfamily #23250

anntzer opened this issue Jun 11, 2022 · 13 comments · Fixed by #24239
Labels
status: has patch patch suggested, PR still needed topic: text/mathtext
Milestone

Comments

@anntzer
Copy link
Contributor

anntzer commented Jun 11, 2022

Bug summary

r"$\|$" should render as a double vertical bar, which it does with usetex or with math_fontfamily="cm", but not with the default "dejavusans" math_fontfamily.

Code for reproduction

s = r"single$|$ double$\|$"
figtext(.5, .7, s); figtext(.5, .6, s, math_fontfamily="cm"); figtext(.5, .5, s, usetex=True)

Actual outcome

test

Expected outcome

Double vertical bar in the first line too.

Additional information

No response

Operating system

arch

Matplotlib Version

3.6.0.dev2423+gd0bef3b3d7

Matplotlib Backend

qt5agg

Python version

3.10

Jupyter version

ENOSUCHLIB

Installation

git checkout

@oscargus
Copy link
Contributor

\Vert seems to work though.

@oscargus
Copy link
Contributor

It is just to change

to

    '|'                        : 8214,

But the base line images are incorrect:
https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/baseline_images/test_mathtext/mathtext_dejavusans_68.png

r"$\left\Vert a \right\Vert \left\vert b \right\vert \left| a \right| \left\| b\right\| \Vert a \Vert \vert b \vert$",

The second b should have double vertical bars around it.

@oscargus oscargus added the status: has patch patch suggested, PR still needed label Jun 11, 2022
@oscargus
Copy link
Contributor

And the problem holds for any other font than cm it seems.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 11, 2022

It would seem reasonable to fix the baseline image (preferably after #22881, as usual: here again, what really matters is that the right glyph gets embedded, not the exact glyph-to-path conversion).

(I haven't checked that the proposed fix works, but it doesn't have to be complicated indeed.)

@oscargus
Copy link
Contributor

I only checked the png output visually. The svg output actually didn't error I realize, so I should probably check that (pdf did error though). Primary problem of fixing it is right now that some of the other pngs error on my machine, so not sure it is a good idea to upload images with slightly off fonts (RMS in the range 1 to 3...).

@oscargus
Copy link
Contributor

And that is because on my Windows machine, the svg tests are not executed. But I can confirm that the PDFs are now correct.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2022

Do you have inkscape installed? If so, svg tests should be executed (if not, please open a separate bug and let's fix that...).

@oscargus
Copy link
Contributor

I have it installed, but, as so often for Windows, not in the PATH. I thought it could be related to something like that. Will try to sort it out.

@oscargus
Copy link
Contributor

Now it breaks SVG as well, so as far as I call tell, it seems to fully work to simply change that.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2022

  1. In the Inkscape installer, is there an option like "add inkscape to PATH"? If so easiest would be to just document that it needs to be installed in this manner.
  2. Otherwise, is Inkscape registered in the registry App Paths subkey (https://docs.microsoft.com/en-us/windows/win32/shell/app-registration#using-the-app-paths-subkey, see also the difference between CreateProcess and ShellExecute)? If so we could read it ourselves to get inkscape's full path. (Note that this is only with inkscape>1.1 (https://gitlab.com/inkscape/inkscape/-/merge_requests/2859) but it seems OK to require that.)

Anyways, this should probably be moved to a separate issue to keep track of it.

@oscargus
Copy link
Contributor

  1. In the Inkscape installer, is there an option like "add inkscape to PATH"?

Yes. I just installed 1.2 to check it, but got an error saying that the PATH was too long to modify. I solved it earlier by simply adding the correct path though.

(I do not think there needs to be an issue for me being lazy/ignorant, or it would be a pretty long issue...)

@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2022

Well, we should at least document that inkscape needs to be installed with that option checked, as that seems like a useful piece of info for other newcomers...

@oscargus
Copy link
Contributor

oscargus commented Aug 20, 2022

Maybe related. Today I realized that autoscaling of | and \| does not work properly.

plt.text(0.05, 0.05, r'$\left|\frac{\frac{3}{2}}{1}\right\|$    $\left(\frac{\frac{3}{2}}{1}\right)$')

image

'cm' seems to work though:

image

Edit: and I realize that my change above could work, but then there are no alternatives so AutoHeightChar breaks on init as char is not set (as alternatives is empty, I assume). If you put 8124 instead of 8214...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: has patch patch suggested, PR still needed topic: text/mathtext
Projects
None yet
3 participants