-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
Travis CI failure seems unrelated since another PR (https://travis-ci.org/matplotlib/matplotlib/jobs/194204607) also fails at the same test. |
lib/matplotlib/font_manager.py
Outdated
@@ -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): |
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.
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.
@NelleV Thank you for the comment. |
…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.
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.
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") |
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 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.
Why are we deprecating this?
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.
Because weight_as_number
is not used by matplotlib after this patch.
Is it acceptable to add the Fira Sans font as a test dependency? |
@kshramt I think we can manage without by checking directly the value yielded by the set_stretch function |
@NelleV I added test cases. |
CI failures seem to be unrelated to this patch. |
Closing reopening for appveyor. |
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.
Not sure who to ping about this weighting thing; @mdboom, maybe?
lib/matplotlib/font_manager.py
Outdated
|
||
# exact match of the weight names (e.g. weight1 == weight2 == "regular") | ||
if (isinstance(weight1, six.string_types) and | ||
isinstance(weight2, six.string_types) and |
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.
Please indent another 4 spaces; this should not be at the same level as the interior block.
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.
Fixed.
fontManager.score_weight("regular", "regular") == | ||
fontManager.score_weight("bold", "bold") < | ||
fontManager.score_weight("normal", "regular") == | ||
fontManager.score_weight(400, 400) < |
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.
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?
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.
This is because "normal" and "regular" have the same numerical weight (
matplotlib/lib/matplotlib/font_manager.py
Lines 99 to 100 in e69ea36
'normal' : 400, | |
'regular' : 400, |
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.
Following on that then, why does 400/400 have a higher score than regular/regular which is the same weight?
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.
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 == |
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.
Can you break this up into a bunch of asserts
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.
Fixed.
…priority (this commit is a partial revert of d94a6a5)
@@ -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] |
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 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.
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.
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.
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.
It seems that the bug here more that 'hair' is getting mapped to the wrong numeric value.
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.
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.
@mdboom is the person who knows the most about font selection. Is there any prior-art for this change to the weight scoring? |
I am not aware of any prior-art for this change. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
…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.