Skip to content
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

ENH: add XKCD colorname -> hex mapping #5775

Merged
merged 11 commits into from Mar 29, 2016

Conversation

tacaswell
Copy link
Member

This still needs

  • smoke test
  • what's new/api changes documentation (re-use table from xkcd color names #4383 )
  • sort out how to make this configurable

Doing this naively with an rcparam is tricky because rcsetup import colors which is imported by __init__.

We could :

  • provide some helper functions in color
  • provide all the X11 names prefixed with 'X11_' so you could be sure to get that if you wanted it
  • do some crazy local imports + hasattr checks

closes #4383

@tacaswell tacaswell added this to the next major release (2.0) milestone Dec 31, 2015
@tacaswell
Copy link
Member Author

There are 100+ test failures from this (which is more like 30-40 unique tests once the pdf/png/svg sets are taken into account) where one of the overlapping colors is used. I am inclined to change the tests to use the 'short' colors which will still have the highest priority.

 - removed darksage, lightsage, sage which are colors we added in
   1ac590e  These colors are now
   included in the XKCD mapping
 - correct sandybrown which mpl had as '#FAA460' but should be '#F4A460'
   according to the w3 group, wikipedia, the copy of rgb.txt bundled
   in the emacs source, and the mapping that we had bundled with AGG for
   a while.  The original value goes back to
   ec45d17
@efiring
Copy link
Member

efiring commented Jan 2, 2016

Minimizing test image replacements this way sounds fine to me.

 - convert the single-letter colors to hex in source
 - single path for looking up string -> hex -> rgb values
@tacaswell
Copy link
Member Author

so

I noticed that there was a lot of missed overlap in color names due to xkcd having space in their names. Fixing that, they basically all clash (cyan, white, and black are the same)

@mdboom
Copy link
Member

mdboom commented Jan 4, 2016

Having so many clashes doesn't really bother me. (i.e. having lots of clashes vs. a few clashes feels like the same about of problem in my mind).

I agree with Eric -- fixing the test source code to force use of the old colors is way preferable over a mass regeneration of images for a sort of "unimportant" (unlikely to uncover bugs) reason.

@tacaswell
Copy link
Member Author

I had not realized initially that the X11 names are also the HTML / CSS
names. I think that is a pretty strong argument for defaulting to those
names over the XKCD names.

On Mon, Jan 4, 2016 at 10:24 AM Michael Droettboom notifications@github.com
wrote:

Having so many clashes doesn't really bother me. (i.e. having lots of
clashes vs. a few clashes feels like the same about of problem in my mind).

I agree with Eric -- fixing the test source code to force use of the old
colors is way preferable over a mass regeneration of images for a sort of
"unimportant" (unlikely to uncover bugs) reason.


Reply to this email directly or view it on GitHub
#5775 (comment)
.

 - prefer the CSS4 str -> hex values because they are the 'standard'.
   The xkcd colors might be 'better', but we are not the right place to
   lead that change.  If users use CSS/HTML/X11 color in mpl they should
   expect it to match the same color set in html for embedding mpl
   figures into websites and such.

 - Provide entries in XKCD dict of all names (with and with out spaces)
   prefixed with XKCD.  Thus to get XKCD's version of blue use
   `color='xkcdblue'
@tacaswell
Copy link
Member Author

The current round of failures is caused by floats not round-tripping to rbg floats, ex

In [15]: import matplotlib.colors as mc

In [16]: mc.hex2color(mc.rgb2hex((0, .5, 0)))
Out[16]: (0.0, 0.5019607843137255, 0.0)

In [17]: mc.hex2color(mc.rgb2hex((0, .75, 0.75)))
Out[17]: (0.0, 0.7490196078431373, 0.7490196078431373)

and in my attempt to simplify handling of the single letter mappings I converted the hard-coded tuples to hard-coded hex values. This is what is causing the tiny differences. Interestingly, I think this mean 'g', 'y', 'm' have always been slightly different than their named counterparts.

The hex representations are not exact which resulted in
very small slight color changes.
@mdboom
Copy link
Member

mdboom commented Jan 18, 2016

I think all we need to ensure is that the precision of the hex numbers (8-bits per plane) is retained. Most of our displays can't display more precision than that anyway.

assert mc.rgb2hex(mc.hex2color(color)) == color

should hold true. But the inverse doesn't really matter.

@tacaswell
Copy link
Member Author

I suspect the failed tests where cases where anti-aliasing was involved and 1 bit got changed.

@tacaswell
Copy link
Member Author

In [116]: ll = [str(i) for i in range(10)] + ['a', 'b', 'c', 'd', 'e', 'f']

In [117]: h = map(lambda x: '#'+''.join(x), itertools.product(*([ll] * 6)))

In [118]: import matplotlib.colors as mc

In [119]: for _ in h:
   .....:     assert _ == mc.rgb2hex(mc.hex2color(_))
   .....: 

This completes sucessfully, but take too long to run to be worth putting in the test suite.

@WeatherGod
Copy link
Member

Yes, 'g', 'y' and 'm' have always been slightly different. I don't know the
full reasons, but it has been that way for as long as I can remember.

On Mon, Jan 18, 2016 at 8:46 PM, Thomas A Caswell notifications@github.com
wrote:

In [116]: ll = [str(i) for i in range(10)] + ['a', 'b', 'c', 'd', 'e', 'f']

In [117]: h = map(lambda x: '#'+''.join(x), itertools.product(*([ll] * 6)))

In [118]: import matplotlib.colors as mc

In [119]: for _ in h:
.....: assert _ == mc.rgb2hex(mc.hex2color(_))
.....:

This completes sucessfully, but take too long to run to be worth putting
in the test suite.


Reply to this email directly or view it on GitHub
#5775 (comment)
.

@jenshnielsen jenshnielsen merged commit 9655f45 into matplotlib:v2.x Mar 29, 2016
@tacaswell tacaswell deleted the enh_xkcd_colors branch March 29, 2016 14:03
@tacaswell tacaswell mentioned this pull request Jan 16, 2017
tacaswell added a commit to QuLogic/matplotlib that referenced this pull request Jan 16, 2017
This dictionary was added in matplotlib#5775 sha: 9655f45 (and not yet released)
and was removed from internal use by matplotlib#6382 sha: (master) 22a7b95 / (2.x
backport) 3281bcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants