/matplotlib

Use xkcd: prefix to avoid color name clashes. #6390

Merged
merged 2 commits into from May 12, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  v2.0.0
4 participants
@anntzer @tacaswell @efiring @mdboom
@anntzer
Contributor

anntzer commented May 9, 2016

Split out of #6382.

Without the prefix, merging CSS4_COLORS and XKCD_COLORS becomes order-dependent.

It's also unclear why space-less names are needed, but I can put them back if someone feels strongly about it...
@anntzer anntzer Use xkcd: prefix to avoid color name clashes. 
Otherwise, merging CSS4_COLORS and XKCD_COLORS becomes order-dependent.

It's also unclear why space-less names are needed, but I can put them
back if someone feels strongly about it...

Some checks were not successful

1 failing and 2 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.005%) to 69.641%
Details
3c1d4e6

mdboom added the needs_review label May 9, 2016

tacaswell added this to the 2.0 (style change major release) milestone May 9, 2016

@tacaswell tacaswell and 1 other commented on an outdated diff May 9, 2016

View full outdated diff
doc/users/colors.rst
All string specifications of color are case-insensitive.
Internally, mpl is moving to storing all colors as RGBA float quadruples.
-
-Name clash between CSS4/X11 and XKCD
@tacaswell

tacaswell May 9, 2016

Owner

Please leave this plot.

@tacaswell

tacaswell May 11, 2016

Owner

Even if we are explicitly name-spacing the colors, I think this chart is still interesting to include in the docs.

@anntzer

anntzer May 11, 2016

Contributor

Are you OK for explicitly namespacing everything then? It'll always be easier to add un-namespaced versions later than removing them...
@tacaswell
Owner

tacaswell commented May 9, 2016

I am still in favor of leaving both the prefixed and un-prefixed versions. For names that are only in the xkcd name space there is no reason to include the extra 5 characters.

We should also document the names as 'XKCD:...' as I think that is his 'preferred' branding.
@anntzer
Contributor

anntzer commented May 9, 2016

I can change the prefix to XKCD, no problem there.
The whole point of the PR is so that {**CSS4_COLORS, **XKCD_COLORS} and {**XKCD_COLORS, **CSS4_COLORS} give the same results, and I'm sort of hoping that this can become an "official" namespacing mechanism for named colors (perhaps seaborn can do its color-remapping by putting its colors in the seaborn-{deep,muted,...}:<...> namespace and we can even have a mechanism to change the named colors resolution order, e.g. plt.colors.set_name_resolution_order(["seaborn-deep", "css4", "xkcd"])).
@tacaswell
Owner

tacaswell commented May 9, 2016

One of the things we decided in the discussion on the pr where the XKCD
colors went in was that we should not change the meaning of the css colors.

On Mon, May 9, 2016, 16:10 Antony Lee notifications@github.com wrote:

I can change the prefix to XKCD, no problem there.
The whole point of the PR is so that {**CSS4_COLORS, **XKCD_COLORS} and {**XKCD_COLORS,
**CSS4_COLORS} give the same results, and I'm sort of hoping that this
can become an "official" namespacing mechanism for named colors (perhaps
seaborn can do its color-remapping by putting its colors in the
seaborn-{deep,muted,...}:<...> namespace and we can even have a mechanism
to change the named colors resolution order, e.g. plt.colors.set_name_resolution_order(["seaborn-deep",
"css4", "xkcd"])).

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6390 (comment)
@anntzer
Contributor

anntzer commented May 9, 2016 • edited

Then the "base" namespace could cover both the one-letter codes and the CSS4 names?
@tacaswell
Owner

tacaswell commented May 9, 2016

I am hesitant to combine the single letter colors (which are home grown)
and the css4 colors (which is a published spec) for the sake of keeping the
code tidy (where tidy here is clarity of what things are) rather than a
minimum number of variables.

The other concern is that we can not encode the single letter colors as hex
due to float rounding issues where as the CSS4 colors are defined as RGB
hex values.

On Mon, May 9, 2016 at 4:37 PM Antony Lee notifications@github.com wrote:

Then "base" could cover both the one-letter codes and the CSS4 names?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6390 (comment)
@anntzer
Contributor

anntzer commented May 9, 2016

TBH I don't feel strongly about anything here, except that having a name clash by default looks... unappetizing. A namespacing/priority mechanism (say, "base", "css4", "xkcd", "seaborn-<...>") would not, by default, change the meaning of the CSS colors (but it would be possible for the user the change them -- in a more "ordered" manner than seaborn currently changes the base colors.
@anntzer
Contributor

anntzer commented May 11, 2016

By the way, regarding xkcd vs XKCD: http://xkcd.com/about/

For those of us pedantic enough to want a rule, here it is: The preferred form is "xkcd", all lower-case. In formal contexts where a lowercase word shouldn't start a sentence, "XKCD" is an okay alternative. "Xkcd" is frowned upon.
@tacaswell
Owner

tacaswell commented May 11, 2016

huh, fair enough re xkcd. Could have sworn I read the opposite....

@tacaswell tacaswell commented on the diff May 11, 2016

View full changes
lib/matplotlib/tests/test_colors.py
@@ -563,7 +563,7 @@ def test_xkcd():
mcolors.colorConverter.to_rgb('blue'))
assert x11_blue == '#0000ff'
XKCD_blue = mcolors.rgb2hex(
- mcolors.colorConverter.to_rgb('XKCDblue'))
+ mcolors.colorConverter.to_rgb('xkcd:blue'))
@tacaswell

tacaswell May 11, 2016

Owner

This does read much better.

@tacaswell tacaswell commented on the diff May 11, 2016

View full changes
lib/matplotlib/_color_data.py
@@ -961,14 +961,9 @@
'green': '#15b01a',
'purple': '#7e1e9c'}
-# normalize to names with no spaces and provide versions with XKCD
-# prefix.
-for k in list(XKCD_COLORS):
- XKCD_COLORS['xkcd'+k] = XKCD_COLORS[k]
- _k = k.replace(' ', '')
- if _k != k:
- XKCD_COLORS[_k] = XKCD_COLORS[k]
- XKCD_COLORS['xkcd'+_k] = XKCD_COLORS[k]
+
+# Normalize name to "xkcd:<name>" to avoid name collisions.
+XKCD_COLORS = {'xkcd:' + name: value for name, value in XKCD_COLORS.items()}
@tacaswell

tacaswell May 11, 2016

Owner

is it worth still providing the space-free versions?

@anntzer

anntzer May 11, 2016

Contributor

I'm not sure what the space-free versions buys you...

@tacaswell

tacaswell May 12, 2016

Owner

slightly terser color names.

@anntzer

anntzer May 12, 2016

Contributor

Can we then just standardize on the space-less versions then? Having both just feels like... not being able to make up your mind. (This would also solve the missing overlaps you mention below.)
@tacaswell
Owner

tacaswell commented May 11, 2016

I am also 👍 on this modulo a few comments.
@anntzer
Contributor

anntzer commented May 11, 2016

I have no idea what's wrong with this build...

@tacaswell tacaswell commented on the diff May 12, 2016

View full changes
doc/users/colors.rst
fig = plt.figure(figsize=[4.8, 16])
ax = fig.add_axes([0, 0, 1, 1])
- j = 0
-
- for n in sorted(overlap, reverse=True):
+ for j, n in enumerate(sorted(overlap, reverse=True)):
@tacaswell

tacaswell May 12, 2016

Owner

This means you can also drop the j += 1 below.

@tacaswell tacaswell commented on the diff May 12, 2016

View full changes
doc/users/colors.rst
import matplotlib.patches as mpatch
- overlap = (set(mcd.CSS4_COLORS) & set(mcd.XKCD_COLORS))
+
+ overlap = {name for name in mcd.CSS4_COLORS
+ if "xkcd:" + name in mcd.XKCD_COLORS}
@tacaswell

tacaswell May 12, 2016

Owner

without the space stripped names you miss some of the overlaps
@tacaswell
Owner

tacaswell commented May 12, 2016

That failure was a bug in pyparsing 2.1.2 which just died on import on 3.4, it is fixed multiple ways already.

@efiring Thoughts on this naming change?

I would be fine if this were merged and it was my job to fix that plot (but would not be opposed to @anntzer fixing it 😉 )
@anntzer anntzer Restore CSS/xkcd comparison plot.

Some checks were not successful

1 failing and 2 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage remained the same at 69.749%
Details
30ffa63
@efiring
Owner

efiring commented May 12, 2016

I like the use of 'xkcd:' as a prefix for all xkcd colors.
I'm confused by the references to "space-stripped" names. The html names are all stripped, but this ugliness is somewhat reduced in http://www.w3schools.com/colors/colors_names.asp by the use of CamelCase. (Leaving out spaces has its advantages, so I'm not complaining about the html names.) The xkcd colors are a mess, with "dark blue" (00035b) and "darkblue" (030764). I suspect whoever thought that was a good idea was high at the time. Overall, the color matching in xkcd is good, but there are too many of them, and too many with revolting names. But we are stuck with that, and I have digressed.
@tacaswell
Owner

tacaswell commented May 12, 2016

I completely missed that there is both 'dark blue' and 'darkblue'. The downsides of machine learning + crowd sourcing....
@anntzer
Contributor

anntzer commented May 12, 2016

Here's the offenders:

In [4]: sorted([c for c in XKCD_COLORS if " " in c and c.replace(" ", "") in XKCD_COLORS])
Out[4]: 
['xkcd:aqua marine',
 'xkcd:blue green',
 'xkcd:blue grey',
 'xkcd:dark blue',
 'xkcd:dark green',
 'xkcd:egg shell',
 'xkcd:golden rod',
 'xkcd:green blue',
 'xkcd:grey blue',
 'xkcd:light blue',
 'xkcd:light green',
 'xkcd:orange red',
 'xkcd:terra cotta',
 'xkcd:yellow green']

Anyways, I guess this answers the question...

@tacaswell tacaswell merged commit d03e798 into matplotlib:master May 12, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 69.749%
Details

tacaswell removed the needs_review label May 12, 2016

@tacaswell
Owner

tacaswell commented May 12, 2016

I will fix that figure my self in a follow on PR

anntzer deleted the anntzer:xkcd-colors-namespace branch May 12, 2016

@tacaswell tacaswell added a commit that referenced this pull request May 12, 2016

@tacaswell tacaswell Merge pull request #6390 from anntzer/xkcd-colors-namespace 
API: Use xkcd: prefix to avoid color name clashes.
 c2b6769
@tacaswell
Owner

tacaswell commented May 12, 2016

backported to 2.x as c2b6769

anntzer referenced this pull request Apr 5, 2017

Open

[feature request] move default colors into styles #8430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment