Skip to content

Created a parameter fontset that can be used in each Text element #18145

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

Merged
merged 2 commits into from
Aug 25, 2020
Merged

Created a parameter fontset that can be used in each Text element #18145

merged 2 commits into from
Aug 25, 2020

Conversation

diegopetrola
Copy link
Contributor

@diegopetrola diegopetrola commented Aug 1, 2020

PR Summary

Closes #17906

  • 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.]

@diegopetrola diegopetrola marked this pull request as draft August 1, 2020 23:01
@anntzer
Copy link
Contributor

anntzer commented Aug 1, 2020

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.

@diegopetrola diegopetrola marked this pull request as ready for review August 3, 2020 20:20
@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 3, 2020

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.

@jklymak 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
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 3, 2020

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.

@jklymak
Copy link
Member

jklymak commented Aug 3, 2020

@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 ;-)

@jklymak jklymak added this to the v3.4.0 milestone Aug 3, 2020
@jklymak
Copy link
Member

jklymak commented Aug 4, 2020

@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.

@diegopetrola
Copy link
Contributor Author

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

@diegopetrola diegopetrola marked this pull request as draft August 5, 2020 01:19
@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 6, 2020

@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).

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

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -364,3 +364,58 @@ def test_mathtext_to_png(tmpdir):
mt = mathtext.MathTextParser('bitmap')
mt.to_png(str(tmpdir.join('example.png')), '$x^2$')
mt.to_png(io.BytesIO(), '$x^2$')


def test_mathtext_fontset():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly stylistic comments.

--------
.text.Text.set_mathtext_fontset
"""
if fontset is None or fontset == '':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@diegopetrola diegopetrola Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@timhoffm
Copy link
Member

timhoffm commented Aug 7, 2020

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.

@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 8, 2020

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

@timhoffm
Copy link
Member

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

@jklymak
Copy link
Member

jklymak commented Aug 15, 2020

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

@QuLogic
Copy link
Member

QuLogic commented Aug 15, 2020

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

@diegopetrola
Copy link
Contributor Author

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! :)

@diegopetrola diegopetrola marked this pull request as draft August 16, 2020 00:44
@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 16, 2020

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.

@diegopetrola
Copy link
Contributor Author

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?

@dopplershift
Copy link
Contributor

You can ignore that macOS failure.

@jklymak
Copy link
Member

jklymak commented Aug 17, 2020

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

@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 17, 2020

@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.

@diegopetrola diegopetrola marked this pull request as ready for review August 18, 2020 23:42
@diegopetrola
Copy link
Contributor Author

diegopetrola commented Aug 23, 2020

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!

fig = plt.figure(figsize=(10, 3))
fig.text(0.2, 0.7, r"$\mathit{This\ text\ should\ have\ one\ font} $",
size=24, math_fontfamily='dejavusans')
fig.text(0.2, 0.3, r"$\mathit{This\ text\ should\ have\ another} $",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
fonts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@diegopetrola diegopetrola Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image
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:
image
If you try to reproduce the same code outside the test environment, no wiggling will occur either:
image

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@diegopetrola diegopetrola Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@jklymak jklymak merged commit 279405d into matplotlib:master Aug 25, 2020
@jklymak
Copy link
Member

jklymak commented Aug 25, 2020

Thanks for all the hard work @diegopetrola

@diegopetrola
Copy link
Contributor Author

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

@isentropic
Copy link

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

@isentropic
Copy link

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

@ain-soph
Copy link
Contributor

ain-soph commented Apr 27, 2021

@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'

@QuLogic
Copy link
Member

QuLogic commented Apr 28, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set mathtext.fontset per element
9 participants