fix cmr10 negative sign in cmsy10 (RuntimeWarning: Glyph 8722 missing) #18397
Conversation
|
@dstansby would you recommend appending to the matplotlib/lib/matplotlib/tests/test_mathtext.py Lines 365 to 372 in aa82b50 Or would you prefer delving into the pytest fixtures? |
|
That would involve changing a test image, which seems unnecessary, so a different (and/or new) test would be better. Also, I don't know why you would have to delve into pytest fixtures? |
|
test failure is real and appears related.... |
|
@jklymak should be fixed now |
|
I'm not sure if I'm doing something wrong, but here is an example where the fix suggested by #17007 (comment) doesn't work: I edited the mentioned lines in Not setting
|
|
AFIACT this is unrelated. Check the value of |
|
What do you mean? For me it is EDIT Oohh, now I get what you mean. With |
|
Is this still an issue? If so this PR needs to pass the tests (preferably rebased on our modern test). |
|
It's still an issue but I've just been manually looping through axes & labels running I already had to change the tests once, not sure if I'll have time to rebase, get familiar with the "modern test"s and update again. |
|
Yes sorry for the process. Things fall off the review queue so unless you ping, folks forget! |
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
I don't know right now, but will throw the ball to gsoc-candidate @aitikgupta :-) |
Co-authored by: Aitik Gupta <aitikgupta@users.noreply.github.com>
|
Hi sorry, because I'm still subscribed to this post I was reminded of it again due to the recent comments. So I thought I just point out one more isse that I ran into when using this fix, and which I forgot to report earlier. For some reason, the degree symbol import matplotlib.pyplot as plt
from matplotlib.ticker import FormatStrFormatter
# plt.rcParams["axes.formatter.use_mathtext"] = True
plt.rcParams["font.family"] = "serif"
plt.rcParams["font.serif"] = "cmr10"
plt.rcParams["mathtext.fontset"] = "cm"
fig = plt.figure()
ax = fig.add_subplot(111, projection='polar')
ax.set_title('Degree °')
# ax.get_xaxis().set_major_formatter(FormatStrFormatter('%d$\,\degree$'))
fig.show()
labels = [label.get_text() for label in ax.get_xticklabels()]
print(labels)
Unlike my previous issue this is not related to the use of mathtext. |
@mapfiable your bug more or less points to the fact that the glyph for |
|
Hi @aitikgupta, thanks, but I think you're mistaken. This thread is dealing with the issue that the minus sign symbol (Glyph 8722) is missing from the cmr10 font and results in the error message pointed out in the title. The use of mathtext is then part of the proposed fix. |
|
From a very quick look (no guarantees...), I agree with @mapfiable here; I believe fixing the degree-sign problem basically requires a similar fix as for the minus sign, i.e. something like As a reminder, the problem is the following:
|
|
I think I misspoke in my statement, I agree with the fundamental issue here - with @mapfiable's example if we do a Axis ticks are wrapped around \mathdefault if we trigger the use_mathtext, however I don't think To get around this, an interesting thing you did was to "force" the formatting to use mathtext, i.e., when you did: ax.get_xaxis().set_major_formatter(FormatStrFormatter('%d$\,\degree$'))However, the font-family in the if condition changed to if font is not None:
+ print("Not found!", font.family_name, uniindex)
glyphindex = font.get_char_index(uniindex)For Overriding a user-defined font shouldn't be the default behaviour right? @anntzer |
|
Could |
Possibly some bad interaction with rcParams["mathtext.fallback"]? (Not sure)
I think this would more or less mean latin modern math (http://www.gust.org.pl/projects/e-foundry/lm-math), which yes, I agree, should be considered as a replacement (I have argued for it elsewhere). |
That's super interesting. I didn't know this was happening at all. In fact, now that you've dissected it like that, I'm not even sure why I was thinking that it should work in the first place.. I guess I thought that EDIT ok just an aside, but something super strage just happened. I could have sworn that ax.get_xaxis().set_major_formatter(FormatStrFormatter('%d$\,\degree$'))did the trick, but I just ran it again and now somehow the values are not in degrees anymore but in multiples of pi (i.e. radians)?! I'm pretty sure this was not the case before because when I came up with the solution I had to run it multiple times to make it work and something this obvious should have caught my eye.. but maybe I'm just going crazy (though I really don't understand why the unit / values should change dependent on whether or not the fmt = lambda x, pos: f'{np.degrees(x):.0f}$\degree$'
ax.get_xaxis().set_major_formatter(FuncFormatter(fmt)) |
|
@aitikgupta If you had a chance to review this, that would be helpful. Thanks! |
There was a problem hiding this comment.
Basically users with font.family == 'cmr10' also need axes.formatter.use_mathtext == True.
(This PR is still required to ensure this works)
I'm not sure if forcing axes.formatter to use mathtext when the font is 'cmr10' is something which should be implemented within the library?
Maybe we'd be better off raising a warning when the font is 'cmr10' and use_mathtext is False (GitHub doesn't allow reviewing on unchanged code):
diff --git a/lib/matplotlib/ticker.py b/lib/matplotlib/ticker.py
--- a/lib/matplotlib/ticker.py
+++ b/lib/matplotlib/ticker.py
@@ -475,6 +475,15 @@ class ScalarFormatter(Formatter):
self._usetex = mpl.rcParams['text.usetex']
if useMathText is None:
useMathText = mpl.rcParams['axes.formatter.use_mathtext']
+ if useMathText is False:
+ for family in mpl.rcParams['font.family']:
+ if "cmr10" in mpl.rcParams[f'font.{family}']:
+ _api.warn_external(
+ 'cmr10 font should ideally be used with '
+ 'mathtext, set axes.formatter.use_mathtext to True'
+ )
+ # or if you *really* want to implement it:
+ # useMathText = True
self.set_useMathText(useMathText)
self.orderOfMagnitude = 0
self.format = ''(could potentially be moved into an outer scope, which includes all formatters and not just ScalarFormatter)
Since this involves something with _api, I believe this needs to go through a senior developer.
Other than this, I think we're good to go 🚀
There was a problem hiding this comment.
I'll approve based on @aitikgupta review. This needs a second review...
|
Note that the actual solution would still require the warning I mentioned in the above comment, to respect one of @anntzer's comments:
Without the warning user wouldn't know that they need to pass |
|
@aitikgupta Can you open a follow on PR with the warning? |
PR Summary
RuntimeWarning: Glyph 8722 missing from current font.Code
Before this PR, this code would not render negative signs on the axes tick labels.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).