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

[with findfont diff] Parsing all families in font_manager by aitikgupta · Pull Request #20496 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

[with findfont diff] Parsing all families in font_manager - #20496

Closed
aitikgupta wants to merge 15 commits into
matplotlib:mainfrom
aitikgupta:font-matching
ConversationCommits (15)ChecksFiles changed
Closed

[with findfont diff] Parsing all families in font_manager#20496
aitikgupta wants to merge 15 commits into
matplotlib:mainfrom
aitikgupta:font-matching

Conversation

aitikgupta commented Jun 23, 2021

Copy link
Copy Markdown
Contributor

PR Summary

This is the beginning of migrating from Matplotlib's "Font-First" approach to a "Text-First" approach.

The very first step is to parse all families in font manager. Previously, we only parsed families until we find a font file, and the rest of the backends are accustomed to just a single font file (which needs changing)

I'm hoping this breaks many tests, so as to know exactly what to fix at other places.

  • Change the return signature of findfont, from simple 'str' to 'OrderedDict'
  • This OrderedDict would contain fonts defined by font.family
    • which are present on the system
    • but also append "fallback fonts" in the end. (which is triggered if a certain font family is not found in the system)

I've added a bunch of To-Dos (future PRs) and some fixes at some places.

PR Checklist

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

aitikgupta commented Jun 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

EDIT: This is solved now

The tests now fail only due to an unrelated issue, and seems to be affecting all recent PRs.

FAILED lib/matplotlib/tests/test_image.py::test_huge_range_log[png] - ValueError: Invalid vmin or vmax

aitikgupta changed the title Parsing all families in font_manager [with findfont change] Parsing all families in font_manager Jun 30, 2021
aitikgupta changed the title [with findfont change] Parsing all families in font_manager [with findfont diff] Parsing all families in font_manager Jun 30, 2021
aitikgupta mentioned this pull request Jun 30, 2021
Closed
7 tasks
jklymak reviewed Jul 4, 2021
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

findfont is public API, so can we just change its return type? At the very least this requires an API change note?

Comment thread examples/misc/logos2.py
# The original font is Calibri, if that is not installed, we fall back
# to Carlito, which is metrically equivalent.
if 'Calibri' in matplotlib.font_manager.findfont('Calibri:bold'):
if 'Calibri' in matplotlib.font_manager.findfont('Calibri:bold').keys():

jklymak Jul 4, 2021

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

I don't think you need .keys()?

aitikgupta Jul 4, 2021

Copy link
Copy Markdown
Contributor Author

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

ah that's right, I was probably searching for findfont usages within mpl and happen to change this eagerly.

aitikgupta commented Jul 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

findfont is public API

Because this is a public API, the return type change is quite huge, which would break things at a lot of places. (including users who don't want their less-relevant part of their work getting messed up, which is also a bit hard to traceback)

I've proposed a 'without findfont diff' parsing of all families, which would only change internals of mpl and won't affect end users. see: #20549

I think this PR should still be open for a while until we've got a good look at the other PR's possibilities.

jklymak commented Jul 4, 2021

Copy link
Copy Markdown
Member

Please reference other relevant PRs in a PR before asking folks to review. I had no idea there were two, and we probably should decide on an approach before reviewing.

jklymak marked this pull request as draft July 4, 2021 21:46

aitikgupta commented Jul 4, 2021

Copy link
Copy Markdown
Contributor Author

Please reference other relevant PRs in a PR before asking folks to review.

I did? I see a aitikgupta mentioned this pull request 5 days ago just above your review @jklymak
(if that's the referencing you meant?) there's also detail about this in the new PR in the Short Note.
Though I apologise if I missed anything.. 😅

On the other hand, the logic still needs to be reviewed (which is mostly similar in this and the other PR), which is also why I didn't convert it into a draft before..

jklymak commented Jul 4, 2021

Copy link
Copy Markdown
Member

Folks aren't going to click through every link on a PR. A quick comment here that this was likely superseded by the other PR would have been helpful.

tacaswell commented Aug 9, 2022

Copy link
Copy Markdown
Member

I am going to close this PR because the core of it got merged in #20740 and we have decided to keep the API on findfont unchanged.

tacaswell closed this Aug 9, 2022
tacaswell added this to the unassigned milestone Aug 9, 2022
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 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL