Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
anntzer merged 2 commits intomatplotlib:masterfromaitikgupta:warn-mathtext
May 25, 2021

Conversation

aitikgupta
Copy link
Contributor

PR Summary

Following up the patch mentioned in#18397 (review), w.r.t.@anntzer's original comment#18397 (comment)

we could even emit a warning when cmr10 is used in a non-mathtext context, as that seems to be pretty common

When using the "cmr10" font (or possibly more mathtext-fonts) for ticks, one should setrcParams["axes.formatter.use_mathtext"] = True to trigger the machinery implemented by#18397

Possible improvements:

  • Move the warning to an outer scope, for all Formatters (not just ScalarFormatter) - do we need to?
  • Include other mathtext-fonts

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak
Copy link
Member

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 putcmr10 in their font family without getting a warning?

@aitikgupta
Copy link
ContributorAuthor

Or are we claiming people can't putcmr10 in their font family without getting a warning?

We're claiming thatcmr10 is not intended for standalone use (many required glyphs are not present in that file but in others), and it only makes sense when used with mathtext.

For ticks, we need to force axes to use mathtext, hence, the warning.

@jklymak
Copy link
Member

Then should there also be a warning when the rcParam is validated?

@aitikgupta
Copy link
ContributorAuthor

By the time we're validating rcParams, I don't think we can identify the use of mathtext (one could explicitly passuseMathText to True and we would still emit the warning if we did this at rcParams level)

@jklymak
Copy link
Member

I guess I'm finding this all a little rickety. When folks wantcmr10 they basically want a suite of LaTeX math fonts? Is there a better strategy here than just special casing sometimes, and not others? Can we takecmr10 to meancmr10+cmbsy10+ etc in general?

@anntzer
Copy link
Contributor

anntzer commentedMay 17, 2021
edited
Loading

Can we take cmr10 to mean cmr10+cmbsy10+ etc in general?

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.

Copy link
Member

@jklymakjklymak left a 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
Copy link
ContributorAuthor

aitikgupta commentedMay 19, 2021
edited
Loading

We would also need to check font.{families}, for example, the repro code at#16995 (comment) before merging.

@jklymakjklymak added this to thev3.5.0 milestoneMay 20, 2021
@jklymak
Copy link
Member

Sorry, are you saying this is not ready for review? I'll move to draft, but feel free to move back

aitikgupta reacted with thumbs up emoji

@jklymakjklymak marked this pull request as draftMay 20, 2021 00:02
@aitikguptaaitikgupta marked this pull request as ready for reviewMay 23, 2021 08:43
@anntzer
Copy link
Contributor

LGTM, but do we want a test?

@aitikgupta
Copy link
ContributorAuthor

Do we test_api warnings?
I could add a simple triggering test in test_ticker.py

@aitikgupta
Copy link
ContributorAuthor

Interesting, this breakstest_usetex_no_warn in test_legend.py:

deftest_usetex_no_warn(caplog):
mpl.rcParams['font.family']='serif'
mpl.rcParams['font.serif']='Computer Modern'
mpl.rcParams['text.usetex']=True
fig,ax=plt.subplots()
ax.plot(0,0,label='input')
ax.legend(title="My legend")
fig.canvas.draw()
assert"Font family ['serif'] not found."notincaplog.text

^when the font isComputer Modern, I'm not entirely sure how to tackle this (maybe change 'Computer Modern' to 'cmr10'?)

mpl.font_manager.FontProperties(
mpl.rcParams["font.family"]
)
) == str(Path(mpl.get_data_path(), "fonts/ttf/cmr10.ttf")):
Copy link
Contributor

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.

@anntzer
Copy link
Contributor

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 tofindfont added by this PR must be wrapped in something that suppresses logging warnings, as we do not want to emit a warning (about using use_mathtext) if the font cannot be found at all (because in that case it certainly doesn't match cmr10).

@aitikgupta
Copy link
ContributorAuthor

aitikgupta commentedMay 24, 2021
edited
Loading

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".
(this happens even if we do absolutely nothing inside it):

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

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 ofScalarFormatter are called)

@anntzer
Copy link
Contributor

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.)
As it turns out findfont uses logging, and suppressing the logwarn is a bit annoying (I guess technically the correct solution would be to temporarily install a logging filter, but ugh); in practice I think the simpler solution may be to callfindfont(..., fallback_to_default=False) which doesn't emit any logwarn, but instead throws a ValueError if no font matches (and then you need to catch that ValueError and treat that as OK, as it means that the user-selected font certainly doesn't match cmr10).

(The font_manager API is turning out to be not really optimal for this use case, but that's another story...)

@aitikgupta
Copy link
ContributorAuthor

Interesting, I started off on the fundamentally wrong track!
I'll update the PR

except ValueError:
ufont = None

if ufont is not None and ufont == str(
Copy link
Contributor

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

Copy link
ContributorAuthor

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 👍🏼

Copy link
Contributor

@anntzeranntzer left a 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.

aitikgupta reacted with thumbs up emoji
@aitikgupta
Copy link
ContributorAuthor

done!

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

@anntzeranntzeranntzer approved these changes

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

3 participants
@aitikgupta@jklymak@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp