The font with the same weight name as the user specified weight name … #7931
Conversation
|
Travis CI failure seems unrelated since another PR (https://travis-ci.org/matplotlib/matplotlib/jobs/194204607) also fails at the same test. |
| return [fname for fname in fontfiles if os.path.exists(fname)] | ||
|
|
||
|
|
||
| def weight_as_number(weight): |
There was a problem hiding this comment.
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.
| return [fname for fname in fontfiles if os.path.exists(fname)] | ||
|
|
||
|
|
||
| @cbook.deprecated("2.1") |
There was a problem hiding this comment.
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. |
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.
Please indent another 4 spaces; this should not be at the same level as the interior block.
| 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.
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.
This is because "normal" and "regular" have the same numerical weight (
matplotlib/lib/matplotlib/font_manager.py
Lines 99 to 100 in e69ea36
There was a problem hiding this comment.
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.
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.
Can you break this up into a bunch of asserts
…priority (this commit is a partial revert of d94a6a5)
| except ValueError: | ||
| if weight not in weight_dict: | ||
| raise ValueError("weight is invalid") | ||
| weight = weight_dict[weight] |
There was a problem hiding this comment.
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.
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.
It seems that the bug here more that 'hair' is getting mapped to the wrong numeric value.
There was a problem hiding this comment.
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_weightimplementation.However, it is clear that if an user set
rcParams["weight"] = "regular", the user wants FiraSans-Regular, not FiraSans-Hair.