-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
@mdehoon Can you please review this? |
Specifically (to @mdehoon) - is there any way you can think of to test this? |
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. |
Well, I hadn't checked at the time but it happens that the As for the suggestion, I'm not sure I understand what you mean: the There is already a test in The thing is I don't know how to force the use of this backend in an automated test; any idea? |
Well, obviously I could do it but I don't understand why. Furthermore, according to the documentation, the values are supposed to follow CSS1 font specification which exclude Finally, as it's an internal value (we're not talking about a value coming from the user), I would check type directly. |
OK.
I'm OK with using int instead of float.
No, but duck-typing is a general style of Python. That said, I can live with using isinstance instead. |
I agree that it would be easier to have the But obviously, it changes the behavior from:
into:
|
I prefer the solution suggested by @fcolas on September 17 rather than the original pull request. |
Ok, I implemented that. |
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 |
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. |
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). |
A grep of I can see and propose to implement several options:
I feel that the less disruptive is the first option, but that goes a bit against the discussion above with @mdehoon. |
At the time of writing |
Now the documentation of 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). |
cc @mdboom |
Do you prefer to do a common fixup of weight-handling in this PR or shall I prepare a own PR for fixing |
@pwuertz Why don't you fix the pgf in it's own PR and then we merge this after that is done.? |
edit: sorry, overlooked the discussion concerning matplotlib diverging from CSS |
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 |
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 ( 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:
edit: numerical value from 100 to 900, not 1000 |
Again, |
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. |
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). |
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. |
Please re-open this targeted against master |
(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 toweight_dict
).