Created a parameter fontset that can be used in each Text element by diegopetrola · Pull Request #18145 · matplotlib/matplotlib · GitHub

PR Summary

Added a keyword argument that can specify the fontset of a Text object. (The paremeter is fontset= or mathtext_fontset=)
The parameter is passed to the FontProperties class, a new variable called self._mathtext_fontset was created to hold this value and the necessary functions to make this work were created.
A pytest function called "test_mathtext_fontset" was created in "test_mathtext.py"
Tests are ok.
Added an example to show the new parameter in action: examples/mathtext_fontset_example.py

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there) [Not major feature.]
Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way [API didn't change in an incompatible way.]

The PR needs a bit of the usual polishing (which I'll let others walk you through ;-)), but I think attaching that info to the FontProperties object (rather than as a separate attribute on the Text) is a really nice idea that makes things much simpler than I would have feared -- from a quick look, I think the basic approach is good.

All the tests that I could do have been done and passed =D
I think the changes are on a good state to be reviewed now.

Thanks for the PR.

I've not read the associated issue. After reading this PR, with particular emphasis on the docs, I have no firm idea what this new kwarg is doing. What happens if I don't set this kwarg? What even is a "fontset"? In the rest of the library we refer to a family and fontproperties. Is this the same? Is fontset just an alias for mathtext_fontset? Why?

I have no firm idea what this new kwarg is doing. What happens if I don't set this kwarg?

If you dont set this kwarg the mathtext will be rendered with the value specified in rcParams['mathtext.fontset']

What even is a "fontset"?

I might be just as lost as you, but I remeber reading somewhere in the docs that a "fontset" is a family of fonts. I will check again to be sure.

Is this the same? Is fontset just an alias for mathtext_fontset? Why?

I just made this for convenience, there are a lot of aliases for the parameters to create a Text like size and fontsize, while studying the creation of a Text class I saw how easy it was to make an alias and, since the name mathtext_fontset seemed too long, I made it. I can revert if you dont think it is necessary.

@DiegoLeal They were more rhetorical questions than suggestions that you change anything, though we may want to do so.

If I understand correctly then I would explain it like this: Matplotlib has a built in math formatter that uses :rc:text.mathtext_fontset to decide what font family to use, and this font family is distinct from the font family used for plain text. This PR allows this "fontset" to be changed on a text-by-text basis, rather than relying on a global value.

I think the todos are:

decide if we want the alias - I'd say no, because it sounds like it applies to non-math fonts as well. If you wanter to go call it math_font I wouldn't object to that.
Document the new kwarg in class FontProperties
Expand on the example by explicitly showing a) that the kwarg doesn't change non-math fonts and b) compare using the rcParam and show how the kwarg overrides the rcParam.

BTW, please feel free to ping me for a re-review. PRs sometimes get buried, and politely asking for us to take a look again is encouraged (within reason ;-)

@DiegoLeal Sorry if I was unclear above - I was only suggesting math_font as an alias instead of fontset. I think I agree with your original choice that all the methods should be called set/get/foo_mathtext_fontset just to be compatible with the existing rcParam.

Ups! =D
I will put it back, indeed the mathtext_fontset looks more comprehensible.

@jklymak I believe the code is in a good state for a review, please take a look when you can :)

Here's a quick preview of what I did:

Improved (a lot!) the example (btw, the idea I had of an example was completely off, I didn't even knew they were integrated to the docs, I am sorry to have wasted your time by making you read through that monstrosity, it should be reasonable now)
Put back the mathtext_fontset param, also made it a getter/setter in the fontProperties. ( Couldn't make it get/set in the Text though, there's a function there that looks for a specific set_foo parameter for every property and will throw a error if it finds none. May I inquire if this was an intentional decision or maybe python just didn't support setters in the past and you had to work around?)
If you dont set any mathtext_fontset param, it will render all text in the last set value of rcParams just as before. (the example has a clear demonstration of this).

Looks close, but quite a few rough edges to get through. Thanks for your patience!

A few seconds? Thats far too long.

I think we need to add an image (just one please) to the repo for this. Then just use our normal figure comparison machinery. If you are going to just compare if two files are equal you can just save to bytesIO instead of to disk. But in this case I think we need the file comparison.

Mostly stylistic comments.

Suggested change

      
                    if fontset is None or fontset == '':
          
                    if fontset is None:

Is there any precedence of supporting an empty string and that being equal to None?

The reason to support fontset=None is to make it more like the other rcParams (and to avoid some bugs), for instance, if I did this:

plt.rcParams['mathtext.fontset'] = 'dejavusans'
plt.text('text1')
plt.rcParams['mathtext.fontset'] = 'dejavuserif'
plt.text('text2')

The texts would have different fonts. I actually liked that change but the initial behavior was not like that. Also, the the interaction with functions like ax.title() and ax.xlabel() would not be correct because the Text object is created before these calls, I would probably need to change ax.title(), ax.xlabel(), ax.ylabel() and probably many others.
By leaving mathtext_fontset = None I ensure the original behavior is maintained, that is: the texts with no math_font will all be rendered with the rcParam present at the moment parse() is called.

I wonder if we should go with math_font (or math_fontfamily) overall. mathtext_fontset feels very bulky. The "text" part is unnecessary because text is implied by "font". I haven't found a strong indication that "fontset" is widely used (e.g. https://en.wikibooks.org/wiki/LaTeX/Fonts does not use the term). It was introduced in 8dc31750 without much explanation.

This would create a naming difference between the rcParam and the property. But now that we're expanding the feature, it may be the right time to turn around and cleanup the naming.

I wonder if we should go with math_font (or math_fontfamily) overall...
...This would create a naming difference between the rcParam and the property

I think this trade off is worth. I will wait some time before making this changes to give people time for a response and avoid having to rename the files again in case we decide for another name.

Also, thanks everyone for your reviews and your time I hope my changes, albeit simple, make matplotlib even better :D

Shouldn't this
git rebase -i issue#17906/master
be
git rebase -i upstream/master

I do a git log and rebase versus the last commit I didn't author as part of the PR.

git rebase -i issue#17906/master
*squashed the commits here*
git push --force origin master

issue#17906 is the name of your branch, and master is the name of another branch. Putting them together doesn't name a revision. As @timhoffm said, you should rebase your branch on upstream/master. You also need to force push your branch, not master, which you haven't change and is why git says "everything up-to-date".

It looks like you squashed some commits, but didn't do a rebase (check git log --graph issue#17906.) Starting after 38c362b, you have one line (ae2a558..682a848) with squashed commits, and one line (5c84526..6f0a9da) with the original commits, and then they're merged together at e7f485b.

To reset to the squashed version of commits:

$ git checkout issue#17906
$ git reset --hard 682a84866774a168c9d5edcfc64a7f44f58c5fa7

then rebase to current:

$ git fetch upstream
$ git rebase -i upstream/master
# change squashing, if you want, but it's already done from the first time you did it

You will get stuck somewhere in between because of the conflicts:

$ $EDITOR lib/matplotlib/mathtext.py
# fix the conflict here, which has to do with a parameter rename in `_parse_cached`
$ git add lib/matplotlib/mathtext.py
$ git rebase --continue

then force push away the merge, and the other set of commits:

$ git push --force origin issue#17906

I think I finally understand (at least part of it), thank you so much @timhoffm and @QuLogic , after countless hours reading docs and tutorials and trying without success to make a repo for testing I was starting to lose hope, I can not thank you enough. I feel reinvigorated to read a bit more about this stuff now, maybe git is not all the bad things I called it in my thoughts. Thank you very much! :)

It seems some pytest errors appeared after I solved the conflict with _force_standard_ps_fonts. The original PR associated with the conflict is closed and all tests have passed (#18002) so my changes must be breaking them somehow.

I started investigating this but have yet to finish.

As I suspected, the errors seems to persist even after I reset my changes (I verified and the errors are the same).
What should I do? Wait untill it gets corrected to test again?

You can ignore that macOS failure.

@DiegoLeal sorry this is such a pain! OTOH, I think you have an empty commit - i.e. 0 files changed now.

@dopplershift thanks for the info!
@jklymak I had reverted the changes to check that the problem was not due to them, I've reset the revertion, they should be back, sorry!

I've put everything in a single commit because I see that's the way it seems to be done by others, I left messages for the big implementations and deleted messages related to debugs and discussion.

Done and done! I also changed the font a little on the tests to make them more distinguishable. The size of the of the baseline image fell from 8 to 5kb, excellent suggestion QuLogic!

I was about to approve, but what the heck is going on with the baseline in the actual image for this font?

Math text is italic by default, right? While implementing my changes I tested different font styles (including the redundant \mathit{}) to be sure they were working properly and I must have forgotten to remove this part. My apologies :)

I wasn't worried about that, but rather the actual text comes out all crooked wrt the baseline. I guess I feel that tests should look right so we can be sure they are testing correct behaviour.

I will look onto that, thx for pointing it out.

I haven't looked carefully but this may just be a very old bug, hinted at in #5414 (comment) (the "wiggly" baselines). IIRC I have a fix somewhere for that, but as usual it never made it even to the PR stage because of course that involves updating all baselines...

I don't think its your changes. OTOH, does all our math look like this or is it just the fact you are calling a sentence?

Yes, it can occur with normal math, but it is only noticeble when there are a lot of letters together, here is one example from baseline_images:

The probable reason there is not a lot of complaints about this is because it's hard to reproduce, if you put on an axis the wiggling will vanish:

If you try to reproduce the same code outside the test environment, no wiggling will occur either:

It might be some setup done by the testing environment, but that's all the info I gathered so far.

So, can we just add the axes? I don't mean to drag you into a really old bug...

I choose a new font that wont display the 'wiggly' behavior. This way the axes wont be needed and the baseline_image size will be kept small. Here is a preview:

Our tests use weird font settings in order to avoid regenerating images. Hopefully that can be dropped soon.

- The parameter can change the family of fonts for each text element instead of globally with `rcParams['mathtext.fontset']` - Added example of usage `/examples/text_labels_and_annotations/mathtext_fontfamily_example.py` - Added tests - Added documentation

The reason for this is to avoid a bug that occurs in testing where the math text will 'wiggle'. This is an old bug in matplotlib not related to the changes done in this PR.

Thanks for all the hard work @diegopetrola

Thanks for all your reviews and guidance @jklymak. You and all the other reviewers who participated here. It was a pleasure.

Hello, I'm the author of the feature request, and I thank the @jklymak and all for making this PR happen. However, under matplotlib 3.3.2

In [5]: matplotlib.__version__
Out[5]: '3.3.2'

I cannot run the example

"""
===============
Math fontfamily
===============
A simple example showcasing the new *math_fontfamily* parameter that can
be used to change the family of fonts for each individual text
element in a plot.
If no parameter is set, the global value
:rc:`mathtext.fontset` will be used.
"""

import matplotlib.pyplot as plt

plt.figure(figsize=(6, 5))

# A simple plot for the background.
plt.plot(range(11), color="0.9")

# A text mixing normal text and math text.
msg = (r"Normal Text. $Text\ in\ math\ mode:\ "
       r"\int_{0}^{\infty } x^2 dx$")

# Set the text in the plot.
plt.text(1, 7, msg, size=12, math_fontfamily='cm')

# Set another font for the next text.
plt.text(1, 3, msg, size=12, math_fontfamily='dejavuserif')

# *math_fontfamily* can be used in most places where there is text,
# like in the title:
plt.title(r"$Title\ in\ math\ mode:\ \int_{0}^{\infty } x^2 dx$",
          math_fontfamily='stixsans', size=14)

# Note that the normal text is not changed by *math_fontfamily*.
plt.show()

errors saying that math_fontfamily is not supported

Nevermind please, I see it is under 3.4 milestone. I wonder when can we approximately expect it to be released?

@diegopetrola
Hi, I recently suffer from the issue caused by math_fontfamily.

If I set the font in text explicitly, then I have to set math_fontfamily as well, otherwise, math_fontfamily will be None and cause exceptions. (even if I set a global default in rcParams['mathtext.fontset'])

Here is a simple reproduce snippet: (it would be working well if you pass the argument math_fontfamily='cm').
I think there should be a default setting if I don't pass math_fontfamily to the Text, and using rcParams['mathtext.fontset'] seems good.

import matplotlib.pyplot as plt
import matplotlib

matplotlib.rcParams['mathtext.fontset'] = 'cm'

plt.figure(figsize=(6, 5))
plt.plot(range(11), color="0.9")
msg = (r"Normal Text. $Text\ in\ math\ mode:\ "
       r"\int_{0}^{\infty } x^2 dx$")
plt.text(1, 7, msg, size=12, font='Arial')  # math_fontfamily='cm'
plt.show()

I check the error and it seems that, the Arial FontProperties passed to that Text is constructed with math_fontfamily=None.

Error log:

Exception in Tkinter callback
Traceback (most recent call last):
  File "C:\Users\ain-s\miniconda3\lib\tkinter\__init__.py", line 1892, in __call__
    return self.func(*args)
  File "C:\Users\ain-s\miniconda3\lib\tkinter\__init__.py", line 814, in callit
    func(*args)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\backends\_backend_tk.py", line 239, in idle_draw
    self.draw()
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\backends\backend_tkagg.py", line 9, in draw
    super().draw()
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\backends\backend_agg.py", line 406, in draw
    self.figure.draw(self.renderer)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\artist.py", line 73, in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\artist.py", line 50, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\figure.py", line 2737, in draw
    mimage._draw_list_compositing_images(
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\image.py", line 132, in _draw_list_compositing_images
    a.draw(renderer)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\artist.py", line 50, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\_api\deprecation.py", line 431, in wrapper
    return func(*inner_args, **inner_kwargs)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\axes\_base.py", line 2925, in draw
    mimage._draw_list_compositing_images(renderer, self, artists)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\image.py", line 132, in _draw_list_compositing_images
    a.draw(renderer)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\artist.py", line 50, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\text.py", line 679, in draw
    bbox, info, descent = textobj._get_layout(renderer)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\text.py", line 314, in _get_layout
    w, h, d = renderer.get_text_width_height_descent(
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\backends\backend_agg.py", line 235, in get_text_width_height_descent
    self.mathtext_parser.parse(s, self.dpi, prop)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\mathtext.py", line 452, in parse
    return self._parse_cached(s, dpi, prop, _force_standard_ps_fonts)
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\mathtext.py", line 461, in _parse_cached
    else _api.check_getitem(
  File "C:\Users\ain-s\miniconda3\lib\site-packages\matplotlib\_api\__init__.py", line 189, in check_getitem
    raise ValueError(
ValueError: None is not a valid value for fontset; supported values are 'cm', 'dejavuserif', 'dejavusans', 'stix', 'stixsans', 'custom'

Please open a separate issue if this is a problem in the current release.

diegopetrola marked this pull request as draft August 1, 2020 23:01

dstansby added the topic: text/fonts label Aug 3, 2020

diegopetrola marked this pull request as ready for review August 3, 2020 20:20

jklymak changed the title Created a parameter fontset that can be used in each Text element (should solve issue#17906). Created a parameter fontset that can be used in each Text element Aug 3, 2020

jklymak reviewed Aug 3, 2020

View reviewed changes

Comment thread lib/matplotlib/font_manager.py Outdated

Comment thread examples/text_labels_and_annotations/mathtext_fontset_example.py Outdated

jklymak added this to the v3.4.0 milestone Aug 3, 2020

jklymak added the status: needs rebase label Aug 3, 2020

diegopetrola marked this pull request as draft August 5, 2020 01:19

diegopetrola marked this pull request as ready for review August 6, 2020 23:10