Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Warn user when mathtext font is used for ticks#20235
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I'm confused by this. I would think you'd only want the warning if there is a minus sign? Or are we claiming people can't put |
We're claiming that For ticks, we need to force axes to use mathtext, hence, the warning. |
Then should there also be a warning when the rcParam is validated? |
By the time we're validating rcParams, I don't think we can identify the use of mathtext (one could explicitly pass |
I guess I'm finding this all a little rickety. When folks want |
anntzer commentedMay 17, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This more or less means (some part of)@aitikgupta's GSOC: currently, non-mathtext textcannot use multiple fonts on a single text object at all. |
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.
Approving as a stop-gap....
aitikgupta commentedMay 19, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We would also need to check font.{families}, for example, the repro code at#16995 (comment) before merging. |
Sorry, are you saying this is not ready for review? I'll move to draft, but feel free to move back |
Uh oh!
There was an error while loading.Please reload this page.
LGTM, but do we want a test? |
Do we test |
Interesting, this breaks matplotlib/lib/matplotlib/tests/test_legend.py Lines 747 to 757 ind56e1b4
^when the font is |
lib/matplotlib/ticker.py Outdated
mpl.font_manager.FontProperties( | ||
mpl.rcParams["font.family"] | ||
) | ||
) == str(Path(mpl.get_data_path(), "fonts/ttf/cmr10.ttf")): |
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.
there's cbook._get_data_path too.
The warning arises because test_legend is using usetex, which uses different family names (here "Computer Modern", not "cmr10"), which can be found by texmanager but not by the normal font system. In practice, what this likely means here is that the call to |
aitikgupta commentedMay 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hmm, something weird is going on with warnings, spent some trying to get this work: withwarnings.catch_warnings():warnings.filterwarnings("ignore")_log.warn("This is not ignored.")# <---ufont=mpl.font_manager.findfont(mpl.font_manager.FontProperties(mpl.rcParams["font.family"]))# ^Neither does it suppress the `findfont` warningifufont==str(cbook._get_data_path("fonts/ttf/cmr10.ttf")):_api.warn_external("cmr10 font should...." ) Moreover, if we use the context manager, the warnings are no longer "just once". withwarnings.catch_warnings():warnings.filterwarnings("ignore")# Do Nothingufont=mpl.font_manager.findfont(mpl.font_manager.FontProperties(mpl.rcParams["font.family"]))ifufont==str(cbook._get_data_path("fonts/ttf/cmr10.ttf")):_api.warn_external("cmr10 font should..." ) ^Output contains a lot of "cmr10 font should..." (since we call multiple instances of Previously, without using the context manager: ufont=mpl.font_manager.findfont(mpl.font_manager.FontProperties(mpl.rcParams["font.family"]))ifufont==str(cbook._get_data_path("fonts/ttf/cmr10.ttf")):_api.warn_external("cmr10 font should..." ) ^Output contains only one "cmr10 font should..." (even when multiple instances of |
I haven't fully unraveled your points, but the fact that warnings.catch_warnings() doesn't catch log.warn() is expected (logging and warnings are two different systems). (But I did miss that initially.) (The font_manager API is turning out to be not really optimal for this use case, but that's another story...) |
Interesting, I started off on the fundamentally wrong track! |
lib/matplotlib/ticker.py Outdated
except ValueError: | ||
ufont = None | ||
if ufont is not None and ufont == str( |
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.
you don't need the None check here
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 was under the impression that we shouldn't compare values if one of them is None...
I'll update 👍🏼
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.
You probably want to squash your commits.
done! |
PR Summary
Following up the patch mentioned in#18397 (review), w.r.t.@anntzer's original comment#18397 (comment)
When using the "cmr10" font (or possibly more mathtext-fonts) for ticks, one should set
rcParams["axes.formatter.use_mathtext"] = True
to trigger the machinery implemented by#18397Possible improvements:
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).