FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

correctly format ticklabels when EngFormatter is used with usetex = True by pharshalp · Pull Request #12847 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

correctly format ticklabels when EngFormatter is used with usetex = True - #12847

Merged
jklymak merged 2 commits into
matplotlib:masterfrom
pharshalp:engformatter_tex_fix
Nov 21, 2018
ConversationCommits (2)ChecksFiles changed
Merged

correctly format ticklabels when EngFormatter is used with usetex = True#12847
jklymak merged 2 commits into
matplotlib:masterfrom
pharshalp:engformatter_tex_fix

Conversation

pharshalp commented Nov 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

ScalarFormatter correctly formats the numbers in ticklabels using $ signs when used with usetex=True but EngFormatter doesn't do so (hence the numbers show up in "text font" instead of "math font"). This results in inconsistent fonts if one of the axes uses EngFormatter but the other one doesn't (see the example below). This PR fixes that by correctly formatting the numbers in ticklabels when EngFormatter is used.

One workaround (as suggested in a related issue #11586 (comment)) is to turn off usetex for the axis that uses ScalarFormatter so that both the axes show ticklables in "text font" (For example -- ax.yaxis.get_major_formatter()._usetex = False).

I think the solution suggested in this PR is much better since it results in consistent (and expected) behavior across different Formatters without having to use the non-public attribute ._usetex.

import matplotlib.pyplot as plt
from matplotlib.ticker import EngFormatter
import numpy as np
plt.rc('text', usetex=True)
plt.rc('font', size=14)

fig, ax = plt.subplots(figsize=(3.0, 2), constrained_layout=True)
ax.plot(np.arange(2000))
ax.set_xticks([0, 500, 1000, 1500, 2000])
ax.set_yticks([0, 500, 1000, 1500, 2000])
formatter = EngFormatter(sep=r'\,')
ax.xaxis.set_major_formatter(formatter)
fig.savefig('test.png', dpi=200)

Before the PR:

After the PR:

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)
  • [ ] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak commented Nov 20, 2018

Copy link
Copy Markdown
Member

I think I agree w. codecov that you should have an image test for this, but others may disagree. But otherwise looks good to me!

pharshalp commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

I have added a test which doesn't rely on image comparison, I hope that's ok.

timhoffm approved these changes Nov 20, 2018
View reviewed changes

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Subject to CI pass.

jklymak approved these changes Nov 20, 2018
View reviewed changes

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Subject to CI

jklymak merged commit f8b425d into matplotlib:master Nov 21, 2018
QuLogic added this to the v3.1 milestone Nov 21, 2018
pharshalp deleted the engformatter_tex_fix branch November 21, 2018 01:36

ImportanceOfBeingErnest commented Nov 21, 2018

Copy link
Copy Markdown
Member

There is a difference between the use of tex for rendering fonts in a figure, which is controlled by the text.usetex rcParam, and the use of mathmode in tex, which, for the ScalarFormatter is controlled by ._usetex attribute.

There is a premise inside the ScalarFormatter that sets those two equal. I.e. it assumes that if you are using tex to render text, you automatically want your labels in mathmode (encapsulated by $).

I do not agree to this premise. Those are really separate things. Now the ScalarFormatter at least allows to set the ._usetex attribute in case you do not want to have your labels in math mode, just because you are using tex.
With this PR, the EngFormatter does not allow to make such choice, since it evaluates the text.usetex rcParam directly.
I think in general one should rather work towards separating those things. I guess that could easily be achieved with two completely independent parameters usetex and usemath.

pharshalp commented Nov 21, 2018

Copy link
Copy Markdown
Contributor Author

I see your point. For now will it be sufficient to add self._usetex = rcParams['text.usetex'] to the __init__ for EngFormatter and use that to give user the choice of setting ._usetex to False?

ImportanceOfBeingErnest commented Nov 21, 2018

Copy link
Copy Markdown
Member

Well, it would be wrong IMO, but at least consistent with ScalarFormatter and hence allow to work around it. So if noone wants to tackle the underlying problem of usetex vs usemath in the near future, I would very much favour the use of something like self._usetex = rcParams['text.usetex'] in the __init__ method.

pharshalp commented Nov 21, 2018

Copy link
Copy Markdown
Contributor Author

ok. I will push a temporary fix (adding self._usetex = rcParams['text.usetex']) for now and try to work on separating the usetex vs usemath logic.

pharshalp mentioned this pull request Nov 21, 2018
Merged
6 tasks

jklymak commented Nov 21, 2018

Copy link
Copy Markdown
Member

I don’t see that adding a private method to give users more control is quite the right solution here!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jklymak jklymak approved these changes
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL