8000 The font with the same weight name as the user specified weight name … by kshramt · Pull Request #7931 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

The font with the same weight name as the user specified weight name … #7931

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 16, 2017

Conversation

kshramt
Copy link
Contributor
@kshramt kshramt commented Jan 23, 2017

…should have the highest priority

FiraSans-Hair and FiraSans-Regular has the same numeric weight, 400, and thus they have the same priority for the previous score_weight implementation.
However, it is clear that if an user set rcParams["weight"] = "regular", the user wants FiraSans-Regular, not FiraSans-Hair.

@kshramt
Copy link
Contributor Author
kshramt commented Jan 23, 2017

Travis CI failure seems unrelated since another PR (https://travis-ci.org/matplotlib/matplotlib/jobs/194204607) also fails at the same test.

NelleV
NelleV previously requested changes Jan 23, 2017
@@ -346,23 +346,6 @@ def findSystemFonts(fontpaths=None, fontext='ttf'):
return [fname for fname in fontfiles if os.path.exists(fname)]


def weight_as_number(weight):
Copy link
Member

Choose a reason for hiding this comment

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

Same as on the other PR, please do not remove functions even if unused. Because Matplotlib is so widely used, we need to go through a deprecation cycle.

@kshramt
Copy link
Contributor Author
kshramt commented Jan 24, 2017

@NelleV Thank you for the comment.
I force pushed the fix.

…should have the highest priority

FiraSans-Hair, which is very thin, and FiraSans-Regular has the same numeric weight, 400, and thus they have the same priority for the previous `score_weight` implementation.
However, it is clear that if an user set `rcParams["weight"] = "regular"`, the user wants FiraSans-Regular, not FiraSans-Hair.
Copy link
Member
@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

Hello,

Sorry it took so long… It seems I had started the review but hadn't finished it.
The patch looks good. Do you think you could add a regression tests to make sure it works as expected?

@@ -346,6 +346,7 @@ def findSystemFonts(fontpaths=None, fontext='ttf'):
return [fname for fname in fontfiles if os.path.exists(fname)]


@cbook.deprecated("2.1")
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Why are we deprecating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because weight_as_number is not used by matplotlib after this patch.

@kshramt
Copy link
Contributor Author
kshramt commented Jan 29, 2017

Is it acceptable to add the Fira Sans font as a test dependency?

@NelleV
Copy link
Member
NelleV commented Jan 29, 2017

@kshramt I think we can manage without by checking directly the value yielded by the set_stretch function

@kshramt
Copy link
Contributor Author
kshramt commented Jan 30, 2017

@NelleV I added test cases.

@kshramt
Copy link
Contributor Author
kshramt commented Feb 1, 2017

CI failures seem to be unrelated to this patch.

@NelleV
Copy link
Member
NelleV commented Feb 2, 2017

Closing reopening for appveyor.

@NelleV NelleV closed this Feb 2, 2017
@NelleV NelleV reopened this Feb 2, 2017
Copy link
Member
@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Not sure who to ping about this weighting thing; @mdboom, maybe?


# exact match of the weight names (e.g. weight1 == weight2 == "regular")
if (isinstance(weight1, six.string_types) and
isinstance(weight2, six.string_types) and
Copy link
Member

Choose a reason for hiding this comment

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

Please indent another 4 spaces; this should not be at the same level as the interior block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

fontManager.score_weight("regular", "regular") ==
fontManager.score_weight("bold", "bold") <
fontManager.score_weight("normal", "regular") ==
fontManager.score_weight(400, 400) <
Copy link
Member

Choose a reason for hiding this comment

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

Why is 400/400 (which are equal) at the same level as normal/regular (which are not equal)? Is it because they're numbers and not strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because "normal" and "regular" have the same numerical weight (

'normal' : 400,
'regular' : 400,
).

Copy link
Member

Choose a reason for hiding this comment

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

Following on that then, why does 400/400 have a higher score than regular/regular which is the same weight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The numerical weight of a font is inferred from the weight name of the font and thus is inaccurate.
For example, FiraSans-Hair, which is very thin, and FiraSans-Regular has the same numeric weight, 400, and thus they have the same priority for the previous score_weight implementation.



def test_score_weight():
assert (0 ==
Copy link
Member

Choose a reason for hiding this comment

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

Can you break this up into a bunch of asserts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -855,7 +847,6 @@ def set_weight(self, weight):
except ValueError:
if weight not in weight_dict:
raise ValueError("weight is invalid")
weight = weight_dict[weight]
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the most concerning change in the PR. Now the type of self._weight is either a string or an int, where as before it was consistently an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Numeric value of self._weight could be inaccurate.
For example, FiraSans-Hair and FiraSans-Regular has the same numeric weight (400), and thus they have the same priority for the previous score_weight implementation.
If we store the weight name of a font instead of the possibly inaccurate numeric weight, we are able to score the font more accurately.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that the bug here more that 'hair' is getting mapped to the wrong numeric value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that "hair" is not a standard weight name, and I do not know a way to map a non-standard weight name to a numeric weight correctly.

@tacaswell
Copy link
Member

@mdboom is the person who knows the most about font selection.

Is there any prior-art for this change to the weight scoring?

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 26, 2017
@kshramt
Copy link
Contributor Author
kshramt commented Feb 26, 2017

I am not aware of any prior-art for this change.

@codecov-io
Copy link
codecov-io commented Feb 26, 2017

Codecov Report

Merging #7931 into master will increase coverage by 43.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #7931       +/-   ##
==========================================
+ Coverage      22%   65.2%   +43.19%     
==========================================
  Files         173     267       +94     
  Lines       56085   69847    +13762     
  Branches        0    9986     +9986     
==========================================
+ Hits        12342   45542    +33200     
+ Misses      43743   21669    -22074     
- Partials        0    2636     +2636
Impacted Files Coverage Δ
lib/matplotlib/font_manager.py 63.29% <100%> (+0.05%)
lib/matplotlib/tests/test_font_manager.py 97.91% <100%> (ø)
lib/matplotlib/widgets.py 49.65% <ø> (-0.7%)
lib/matplotlib/backends/qt_editor/figureoptions.py 0% <ø> (ø)
lib/matplotlib/backends/qt_editor/formlayout.py 0% <ø> (ø)
lib/matplotlib/backend_tools.py 30.91% <ø> (ø)
lib/mpl_toolkits/axisartist/grid_finder.py 0% <ø> (ø)
lib/matplotlib/docstring.py 90.47% <ø> (ø)
lib/matplotlib/backends/backend_webagg.py 0% <ø> (ø)
lib/mpl_toolkits/axisartist/angle_helper.py 0% <ø> (ø)
... and 195 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5652c66...42b1659. Read the comment docs.

@anntzer anntzer dismissed NelleV’s stale review April 16, 2017 05:26

issue was addressed

@anntzer anntzer changed the title The font with the same weight name as the user specified weight name … [MRG+1] The font with the same weight name as the user specified weight name … Apr 16, 2017
@tacaswell tacaswell merged commit 249f7bb into matplotlib:master Apr 16, 2017
@tacaswell
Copy link
Member

Thanks @kshramt !

Merged with some reservations, we should keep this in mind for the future if we get any more weird font selection bug reports.

@QuLogic QuLogic changed the title [MRG+1] The font with the same weight name as the user specified weight name … The font with the same weight name as the user specified weight name … Apr 16, 2017
@tacaswell tacaswell mentioned this pull request May 1, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0