Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 21, 2018
Merged

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

merged 2 commits into from
Nov 21, 2018

Conversation

pharshalp
Copy link
Contributor

@pharshalp pharshalp commented Nov 20, 2018 •
edited
Loading

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:

old

After the PR:

new

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

jklymak commented Nov 20, 2018

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
Copy link
Contributor Author

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

timhoffm
timhoffm approved these changes Nov 20, 2018
View reviewed changes
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.

Subject to CI pass.

jklymak
jklymak approved these changes Nov 20, 2018
View reviewed changes
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.

Subject to CI

@jklymak jklymak merged commit f8b425d into matplotlib:master Nov 21, 2018
@QuLogic QuLogic added this to the v3.1 milestone Nov 21, 2018
@pharshalp pharshalp deleted the engformatter_tex_fix branch November 21, 2018 01:36
@ImportanceOfBeingErnest
Copy link
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
Copy link
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
Copy link
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
Copy link
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 pharshalp mentioned this pull request Nov 21, 2018
Merged
6 tasks
@jklymak
Copy link
Member

jklymak commented Nov 21, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm 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
@pharshalp @jklymak @ImportanceOfBeingErnest @timhoffm @QuLogic