-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Add feature to fallback to stix font in mathtext #11644
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
Conversation
- Add new rcParam `mathtext.fallback` - Update UnicodeFonts class to support fallback to stix
himbeles
commented
Jul 12, 2018
Hi,
seems to be working nicely for me except for "scaled" brackets using \left and \right TeX syntax with stix.
Minimum failing and working examples:
import matplotlib as mpl import matplotlib.pyplot as plt # MATH FONT mpl.rcParams['mathtext.fontset'] = 'custom' mpl.rcParams['mathtext.rm'] = 'Helvetica Neue' mpl.rcParams['mathtext.it'] = 'Helvetica Neue:italic' mpl.rcParams['mathtext.bf'] = 'Helvetica Neue:bold' test_str1 = r'$\left(a\right) \rightarrow 1$' test_str2 = r'$a \rightarrow 1$' def test_plot(test_str): fig = plt.figure(figsize=(7,5)) plt.title(mpl.rcParams['mathtext.fallback']) plt.plot([0,2],[0,2]) plt.text(1,1,test_str, fontsize=20) plt.show() # working example 1 mpl.rcParams['mathtext.fallback'] = "cm" test_plot(test_str1) # working example 1 mpl.rcParams['mathtext.fallback'] = "stix" test_plot(test_str2) # failing example mpl.rcParams['mathtext.fallback'] = "stix" test_plot(test_str1)
himbeles
commented
Jul 12, 2018
@nelsond 👍 Working great, thank you!
lib/matplotlib/mathtext.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could just set fontname='rm'
in this line, get rid of the else block below, and then just have a single return self.cm_fallback._get_glyph(fontname, font_class, sym, fontsize)
instead of three.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I fixed this a couple of days ago.
lib/matplotlib/mathtext.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(削除) I would directly write this as a list See dict suggestion below.stixsizedaltfonts = ["STIXGeneral", ...]
.
It's more readable than splitting a string. Also, you can wrap within the list and don't need the ugly line continuation char 😄. (削除ここまで)
lib/matplotlib/mathtext.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the numbering is essential, have you thought about turning stixsizedaltfonts
into a dict, similar to what is done in DejaVuFonts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for splitting a string here because that is also done in line 770.
for texfont in "cal rm tt it bf sf".split(): # ...
But of course, there is no line break and the order does not matter. So, I agree that a dict is more appropriate here. I also made sure to add the STIX alternatives with their names to the fontmap
self.fontmap[name] = fullpath
Not sure whether this is really necessary, but it is also the case for the DejaVuFonts
.
lib/matplotlib/rcsetup.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid values of s
must be a string. This check can be left out and you can just if s.lower() in fallback_fonts:
. If you want to provide a better error message in case the user passes some other object, it's better to explicitly
if not isinstance(s, str):
raise ValueError("'mathtext.fallback' must be a string.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this. Not sure, I understand this completely, since validate_fontsize
has a similiar check/conversion. But maybe I am missing a subtle point here.
def validate_fontsize(s): fontsizes = ['xx-small', 'x-small', 'small', 'medium', 'large', 'x-large', 'xx-large', 'smaller', 'larger'] if isinstance(s, str): s = s.lower() if s in fontsizes: return s try: return float(s) except ValueError: raise ValueError("%s is not a valid font size. Valid font sizes " "are %s." % (s, ", ".join(fontsizes)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate_fontsizes
does also accept numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see. But I might want to go with your initial suggestion then. Currently, the following code
mpl.rcParams['mathtext.fallback'] = 1
leads to the error
AttributeError: 'int' object has no attribute 'lower'
Of course the user should only supply a string, but the error message might not be very helpful in finding a potential bug in your own code accessing mpl.rcParams['mathtext.fallback']
.
I added the following
if not isinstance(s, str): raise ValueError("Must be a string or None.")
which yields the following error message
ValueError: Key mathtext.fallback: Must be a string or None.
I hope that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some documentation. In particular it seems fallback
overrides fallback_to_cm
if its set; there should be a comment in here.
Preferably, we would deprecate fallback_to_cm
and make cm
the default for mathtext.fallback
.
This needs to be in matplotlibrc.template
as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STIX? (typo)
Sorry about opening #16492. Having this up would be great :)
@andrzejnovak Do you think this should also support fallback = dejavusans/... (basically anything supported by mathtext.fontset) as suggested in #8619? Would you like to pick up this PR and manage the rebase and push it through the finish line? I think there are the review comments to address, at least a little bit of doc (it's pretty obscure...), perhaps deprecate fallback_to_cm?, and possibly support the other fallbacks as well.
@anntzer Seems what's missing is largely cosmetic changes, I could take a look. In principle, this should imo support any fallback font if available, such as Fira Math. But the current scope would already be enough of a fix for me.
Go for it :)
This is replaced by #16509, isn't it?
AFAICT, yes.
Thanks for the PR :)
PR Summary
When using a custom font for math (
mathtext.fontset : custom
), the only fallback for undefined symbols/characters is Computer Modern at the moment. See also #8619.The pull request contains a new rcParam
mathtext.fallback
which can bestix
,cm
or None.Support for the original rcParam
mathtext.fallback_to_cm
is still available. The feature seems to work as the plots shows.stix cm
(Example plot with STIX (left) respectively CM (right) as mathtext fallback and Helvetica Neue as custom mathtext font)
If this pull request has any chance of getting merged, I'm happy to clean up the code, add documentation, and testing.
PR Checklist