Skip to content

Fix #3330 by changing weight into string [cherry-pick to master] #3334

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

Closed
wants to merge 1 commit into from

Conversation

fcolas
Copy link

@fcolas fcolas commented Jul 31, 2014

(see issue #3330).
The native part of the backend is actually checking whether the weight is "bold" (and the rest is normal) (see _macosx.m:setfont).
So I'm checking weight against 700 and I'm putting bolder weights to "bold" (according to weight_dict).

@tacaswell
Copy link
Member

@mdehoon Can you please review this?

@tacaswell tacaswell added this to the v1.4.x milestone Jul 31, 2014
@matthew-brett
Copy link
Contributor

Specifically (to @mdehoon) - is there any way you can think of to test this?

@mdehoon
Copy link
Contributor

mdehoon commented Aug 13, 2014

Overall the fix looks good to me, but what if weight is a floating-point value? If the weight is allowed to be a float, then isinstance(weight, numbers.Number) would be better.
I would suggest to use test_text.py, and try if boldFont is created correctly if the weight argument is a numerical value.

@fcolas
Copy link
Author

fcolas commented Aug 19, 2014

Well, I hadn't checked at the time but it happens that the set_weight method of FontProperty conveniently casts the value it receives into an int (or checks it's a known weight name such as 'bold' or 'normal').
Thus, when the code I propose receives the weight value, it's either an int or a string.

As for the suggestion, I'm not sure I understand what you mean: the FontProperty object was always created correctly but, at drawing time (calls to gc.get_text_width_height_descent and gc.draw_text), there would be an exception because the backend expects a string whereas the weight can be either string or int.

There is already a test in test_text.py that creates a bold text using numerical value (750 by the way, that's not allowed by the CSS spec linked in the docstring) but it doesn't fail as it doesn't use the macosx backend as far as I understand.

The thing is I don't know how to force the use of this backend in an automated test; any idea?

@fcolas
Copy link
Author

fcolas commented Sep 14, 2014

Well, obviously I could do it but I don't understand why.
As far as I can see, the value is already sanitized by the set_weight method and cast into either an int if it's numerical or a string.
Am I missing something?

Furthermore, according to the documentation, the values are supposed to follow CSS1 font specification which exclude float values (only multiples of 100 are allowed as numerical values), so I wouldn't choose float but int.

Finally, as it's an internal value (we're not talking about a value coming from the user), I would check type directly.
Is the general style of matplotlib with internal union types to use the exception mechanism to check the type rather than checking it directly?

@mdehoon
Copy link
Contributor

mdehoon commented Sep 16, 2014

As far as I can see, the value is already sanitized by the set_weight method and cast into
either an int if it's numerical or a string. Am I missing something?

OK.
One thing I am wondering about though: Wouldn't it be better for set_weight to always store the weight as an integer? It seems that now each of the backends is checking whether weight is a string or an integer, whereas we could do that in one central place in set_weight. Then get_weight always returns an integer.

Furthermore, according to the documentation, the values are supposed to follow CSS1 font
specification which exclude float values (only multiples of 100 are allowed as numerical values), so I > wouldn't choose float but int.

I'm OK with using int instead of float.

Finally, as it's an internal value (we're not talking about a value coming from the user), I would
check type directly.
Is the general style of matplotlib with internal union types to use the exception mechanism to check
the type rather than checking it directly?

No, but duck-typing is a general style of Python. That said, I can live with using isinstance instead.

@fcolas
Copy link
Author

fcolas commented Sep 16, 2014

I agree that it would be easier to have the _weight private attribute to always be an int.
If everybody agree, I can propose a patch for that based on the weight_as_number function and then simplify the fix above accordingly.
It shouldn't break the behavior of other backends but they should still be adapted as well.

But obviously, it changes the behavior from:

>>> fp.set_weight('normal')
>>> fp.get_weight()
'normal'

into:

>>> fp.set_weight('normal')
>>> fp.get_weight()
400

@tacaswell
Copy link
Member

@mdehoon @fcolas What is the state of this PR?

@mdehoon
Copy link
Contributor

mdehoon commented Oct 22, 2014

I prefer the solution suggested by @fcolas on September 17 rather than the original pull request.

@fcolas
Copy link
Author

fcolas commented Oct 22, 2014

Ok, I implemented that.
Now, I've updated the macosx backend to assume that it gets its weight as an int (so as to actually solve #3330) but I didn't check if we could simplify the code of other backends.

@tacaswell
Copy link
Member

This breaks the pgf backend (which is expecting a string).

This is too big of a change to go in the 1.4.x branch as-is and needs to be documented in api_changes.rst.

attn @pwuertz

@tacaswell tacaswell changed the title Fix #3330 by changing weight into string Fix #3330 by changing weight into string [cherry-pick to master] Oct 28, 2014
@fcolas
Copy link
Author

fcolas commented Oct 29, 2014

Correct me if I'm wrong, but pgf shouldn't expect a string since the weight can be either a string or an int (before my patch). That's the same issue as with the macosx backend that we're trying to fix here.

At that point, I think we should check all backends to see which of them currently fully support both string and int. Those wouldn't be broken by my patch, the others are incomplete and should be fixed one way or another.

I'm ready to help on that (once a strategy is decided) but my knowledge of backends and their testing is lacking.

@tacaswell
Copy link
Member

You are correct pgf should handle both ints and strings, but it does not (see https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/backend_pgf.py#L165).

@fcolas
Copy link
Author

fcolas commented Oct 30, 2014

A grep of get_weight in the backends/ directory shows that it's used in cairo, gdk, macosx, pgf, and wx (the rest don't have weight either).
In cairo, gdk, and wx, it's used with a fontweight dictionary having both integer and string keys.
Only macosx and pgf seem to assume a string weight.

I can see and propose to implement several options:

  • replicate what is done in cairo, gdk, and wx backends in macosx and pgf so as to have them handle both string and int (meaning we forget about this current patch).
  • keep the current patch and fix pgf to accept (only) int.
  • keep the current patch, change cairo, gdk, pgf, and wx to accept only int.

I feel that the less disruptive is the first option, but that goes a bit against the discussion above with @mdehoon.

@pwuertz
Copy link
Contributor

pwuertz commented Oct 30, 2014

At the time of writing backend_pgf I found some part of the documentation listing only the weight strings. The fontweight dicts you mentioned only cover specific numeric values, what happens if the fontweight is 901 ? Is there a description for these numeric values?

@fcolas
Copy link
Author

fcolas commented Oct 30, 2014

Now the documentation of FontProperty states that it follows the CSS level 1 spec (with a link to the level 2 spec, by the way ^_^).
The part of this spec about font weight (which is the same in both versions) allows the following list of values: normal, bold, bolder, lighter, 100, 200, 300, 400, 500, 600, 700, 800, and 900.
So 901 is an incorrect value.
Note that we diverge from the CSS spec already by not supporting lighter and bolder (which are relative to the inherited value) and allowing a lot more strings such as light or heavy (used by gdk at least).

As to what those means, the spec gives clear heuristics. For example, if you only have normal and bold, 100 to 500 will be normal and 600 to 900 will be bold (which means my patch above is wrong as it checks against 700 instead of 600).

@tacaswell
Copy link
Member

cc @mdboom

@pwuertz
Copy link
Contributor

pwuertz commented Oct 30, 2014

Do you prefer to do a common fixup of weight-handling in this PR or shall I prepare a own PR for fixing pgf_backend ?

@tacaswell
Copy link
Member

@pwuertz Why don't you fix the pgf in it's own PR and then we merge this after that is done.?

@pwuertz
Copy link
Contributor

pwuertz commented Oct 31, 2014

edit: sorry, overlooked the discussion concerning matplotlib diverging from CSS

@pwuertz
Copy link
Contributor

pwuertz commented Oct 31, 2014

No, sorry, I'm still confused. Are the backends supposed to follow the matplotlib definition? If so, "range 0-1000" clearly includes values like 0 and 901, or 9.1 which requires some fixing in other backends as well. If we are to follow the linked CSS definitions then weights like ultralight or semibold do not apply anymore.

@pwuertz
Copy link
Contributor

pwuertz commented Oct 31, 2014

This seems to be way more difficult than expected. Matplotlib defines a total of 14 named weights. This is quite a lot. For example, the fontweights dict in the wx backend you mentioned does not cover all of them (it would choke on demibold for example) and accepts another weight (ultrabold) which is not mentioned in matplotlib.
In the end, the set of possible weight-names and their corresponding font weight is entirely dependent on the backend and varies for each framework. So how to specify a font-weight in matplotlib, independent of the backend then?
It seems that this large list in the matplotlib documentation tries to cover all definitions from many backends, but this is really difficult to implement, since each backend would have to support and interpret all weight-names from all other backends as well - for every new name from every new backend as well.
The numeric values could be a way to make font-weight specification more portable, but those don't seem to be standardized either. Still, its better than trying to figure out the difference between demi, semibold and demibold and how those are to be mapped to your backend-specific font-engine.

So, anyone up for a re-definition of font-weights in matplotlib? I'd propose to follow CSS since it's already linked in the documentation. So a weight could be:

  • A string from a minimal list anyone can agree on, supported by all backends: normal | bold | bolder | lighter.
  • A numerical value from 100 to 900 in steps of 100. Results may vary.
  • Any other string is also valid but matplotlib does not guarantee that it works on all backends. In fact, specifying anything else implies that a user is aware of which backend he has chosen.

edit: numerical value from 100 to 900, not 1000

@fcolas
Copy link
Author

fcolas commented Oct 31, 2014

Again, bolder and lighter have a meaning in CSS that might not be relevant in matplotlib as they specifically refer to the next darker or lighter face than the inherited value. If we want to support these values, we need to be explicit in what they do.
But other than that, your proposal seems fair.

@tacaswell
Copy link
Member

Would it make more sense to map ranges of integers to weights?

On the front end we provide string -> int conversion for the intersection of weights/some sub set of the union, internally store the the weight as an integer and then all of the backends convert that integer into what ever backend-specific thing they can deal with by binning.

@pwuertz
Copy link
Contributor

pwuertz commented Oct 31, 2014

Working with absolute numbers would make sense indeed, but these integer font weights look pretty much arbitrary to me. Most backends would have to map them to some font weight in accordance to the author's gut feeling. Some mappings wouldn't be possible at all. In principal, latex allows for four weights lighter than normal, whereas that number system only allows for three. On the bold side, there are more numbers than weight specifications. How to map these? Does ultra-bold start at 700 or at 800? How could a user ever guess which number or independently defined weight-language would map to a specific weight specification of a backend?

I don't know how this discussion started, but do we need the numbers in the frontend? How about only defining "normal", "bold" and "light" as universally recognized weights (all backends must understand those) and leave any other value up to the backend's interpretation (backends must ignore unknown values without raising exceptions).

@mdboom
Copy link
Member

mdboom commented Oct 31, 2014

The numbers go back to the Postscript Type 1 standard, but are still used in more modern technologies like CSS 3. This is how the weight is specified in most TrueType fonts (and are considered generally more accurate than any of the string-based names in the font file). Values that are not an even multiple of 100 are not valid and do not need to be supported -- I think that's a documentation bug in matplotlib that it suggests they are supported -- the font cache does not work that way.

@tacaswell tacaswell modified the milestones: v1.4.x, 1.5.0 Feb 7, 2015
@tacaswell tacaswell closed this Feb 17, 2015
@tacaswell
Copy link
Member

Please re-open this targeted against master

@tacaswell tacaswell mentioned this pull request Feb 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants