The font with the same weight name as the user specified weight name … by kshramt · Pull Request #7931 · matplotlib/matplotlib

kshramt · 2017-01-23T09:34:48Z

…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 · 2017-01-23T10:08:47Z

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

NelleV · 2017-01-23T16:40:42Z

    return [fname for fname in fontfiles if os.path.exists(fname)]


-def weight_as_number(weight):


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 · 2017-01-24T02:30:47Z

@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.

NelleV

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?

NelleV · 2017-01-24T02:58:44Z

    return [fname for fname in fontfiles if os.path.exists(fname)]


+@cbook.deprecated("2.1")


Why are we deprecating this?

Because weight_as_number is not used by matplotlib after this patch.

kshramt · 2017-01-29T07:21:23Z

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

NelleV · 2017-01-29T17:58:14Z

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

kshramt · 2017-01-30T12:30:53Z

@NelleV I added test cases.

kshramt · 2017-02-01T09:13:34Z

CI failures seem to be unrelated to this patch.

NelleV · 2017-02-02T00:33:01Z

Closing reopening for appveyor.

QuLogic

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

QuLogic · 2017-02-25T09:34:27Z

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


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

QuLogic · 2017-02-25T09:36:12Z

+            fontManager.score_weight("regular", "regular") ==
+            fontManager.score_weight("bold", "bold") <
+            fontManager.score_weight("normal", "regular") ==
+            fontManager.score_weight(400, 400) <


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?

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,

).

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

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.

- matplotlib#7931 (comment) - matplotlib#7931 (comment)

tacaswell · 2017-02-26T03:36:45Z

+
+
+def test_score_weight():
+    assert (0 ==


Can you break this up into a bunch of asserts

…priority (this commit is a partial revert of d94a6a5)

tacaswell · 2017-02-26T03:42:12Z

        except ValueError:
            if weight not in weight_dict:
                raise ValueError("weight is invalid")
-            weight = weight_dict[weight]


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.

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.

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

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 · 2017-02-26T03:46:34Z

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

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

kshramt · 2017-02-26T05:22:43Z

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

codecov-io · 2017-02-26T06:06:03Z

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.

issue was addressed

tacaswell · 2017-04-16T19:07:39Z

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.

NelleV previously requested changes Jan 23, 2017

View reviewed changes

kshramt force-pushed the font_weight branch from ad11c5e to 818443f Compare January 24, 2017 02:28

kshramt force-pushed the font_weight branch from 818443f to d94a6a5 Compare January 24, 2017 02:41

NelleV reviewed Jan 27, 2017

View reviewed changes

Add tests

30031b5

kshramt force-pushed the font_weight branch from 147890e to 30031b5 Compare January 30, 2017 12:26

NelleV closed this Feb 2, 2017

NelleV reopened this Feb 2, 2017

QuLogic reviewed Feb 25, 2017

View reviewed changes

Address comments

97e7dac

- matplotlib#7931 (comment) - matplotlib#7931 (comment)

tacaswell reviewed Feb 26, 2017

View reviewed changes

An inferred weight should be stored as a numeric value to gain lower …

8a10efc

…priority (this commit is a partial revert of d94a6a5)

tacaswell reviewed Feb 26, 2017

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Feb 26, 2017

kshramt added 3 commits February 26, 2017 13:09

Address a comment

91075a3

Remove a test following 8a10efc

a7026e0

PEP8

42b1659

anntzer approved these changes Apr 16, 2017

View reviewed changes

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 merged commit 249f7bb into matplotlib:master Apr 16, 2017

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 mentioned this pull request May 1, 2017

Improve font weight guessing #8551

Closed

1 of 5 tasks

tacaswell mentioned this pull request Jun 22, 2017

Font issue: findfonts should differentiate between thin and regular ttf fonts #8788

Closed

		return [fname for fname in fontfiles if os.path.exists(fname)]


		def weight_as_number(weight):

		return [fname for fname in fontfiles if os.path.exists(fname)]


		@cbook.deprecated("2.1")

Conversation

kshramt commented Jan 23, 2017

Uh oh!

kshramt commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kshramt commented Jan 24, 2017

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kshramt commented Jan 29, 2017

Uh oh!

NelleV commented Jan 29, 2017

Uh oh!

kshramt commented Jan 30, 2017

Uh oh!

kshramt commented Feb 1, 2017

Uh oh!

NelleV commented Feb 2, 2017

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Feb 26, 2017

Uh oh!

kshramt commented Feb 26, 2017

Uh oh!

codecov-io commented Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tacaswell commented Apr 16, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Feb 26, 2017 •

edited

Loading