Adding names to the color in the new Vega10 color cycle by trpham · Pull Request #7343 · matplotlib/matplotlib

trpham · 2016-10-25T05:43:30Z

Prefer to this ticket: #7248 "Adding names to the color in the new (Vega) default color cycle"

Implement VEGA10_COLORS dictionary package in _color_data.py,
Use name colors, prefix vega10:__ (where __ is blue, orange, green, red, purple, brown, pink, gray, olive, cyan), respectively equivalent to C0, C1,..., C9
In colors.py, import VEGA10_COLORS, and add this new dictionary into _colors_full_map

NelleV · 2016-10-25T15:49:18Z

Hi @trpham
When submitting a pull request, please add a description of the feature added or the bug fixed.
Else, the reviewers have no clue what the patch proposes.

NelleV

In terms of code, this is mostly good (apart from the pep8 violation).

We are missing a test: Look at lib/matplotlib/tests/test_colors.py l: 575 how it's done for the xkcd.

I'd also like to have a bit more documentation, but I haven't figured out a good way to do this, so I'll get back to you on that.

NelleV · 2016-10-26T17:35:08Z

doc/users/colors.rst

 * a name from the `xkcd color survey <https://xkcd.com/color/rgb/>`__
  prefixed with ``'xkcd:'`` (e.g., ``'xkcd:sky blue'``)
 * one of ``{'C0', 'C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8', 'C9'}``
+* one of ``{'blue', 'orange', 'green', 'red', 'purple', 'brown', 'pink', 'gray', 'olive', 'cyan'}``


Thanks for thinking about documenting this change! That's a really important part of making new features accessible to users.

I think you should add the "vega" prefix to be more specific.

NelleV · 2016-10-26T17:39:53Z

lib/matplotlib/_color_data.py

+
+
+# Normalize name to "vega:<name>" to avoid name collisions.
+VEGA10_COLORS = {'vega:' + name: value for name, value in VEGA10_COLORS.items()}


That line is slightly too long for pep8 (1 character too long). The tests are failing because of this line.

@NelleV I did notice that, but it's exactly 80 characters, and I thought leave it like that way is cleaner. Let's me fix it.

NelleV · 2016-10-26T17:40:03Z

lib/matplotlib/colors.py

    """
    def __init__(self, name, N=256):
-        r"""
+        """


phobson · 2016-10-26T18:05:49Z

lib/matplotlib/_color_data.py

+    'pink': '#e377c2',
+    'gray': '#7f7f7f',
+    'olive': '#bcbd22',
+    'cyan': '#17becf'}


pep8 might want that closing brace to be on the next line. Not sure though.

It doesn't. That line is fine pep8-wise.

… pep8 style for VEGA10_COLORS dictionary in _color_data.py

NelleV · 2016-10-26T18:35:05Z

lib/matplotlib/colors.py


 cnames = CSS4_COLORS
-COLOR_NAMES = {'xkcd': XKCD_COLORS, 'css4': CSS4_COLORS}
+COLOR_NAMES = {'xkcd': XKCD_COLORS, 'css4': CSS4_COLORS, 'vega': VEGA10_COLORS}


The 10 is missing in the name: it should be 'vega10'

NelleV · 2016-10-26T18:35:24Z

lib/matplotlib/_color_data.py

+    'cyan': '#17becf'}
+
+
+# Normalize name to "vega:<name>" to avoid name collisions.


I think we decided on 'vega10' as the prefix.

I don't think there was a consensus on that (and I'm not sure it's necessary either.)

… in colors.py

NelleV · 2016-10-27T12:09:55Z

doc/users/colors.rst

 * a name from the `xkcd color survey <https://xkcd.com/color/rgb/>`__
  prefixed with ``'xkcd:'`` (e.g., ``'xkcd:sky blue'``)
 * one of ``{'C0', 'C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8', 'C9'}``
+* one of ``{'vega10:blue', 'vega10:orange', 'vega10:green', 'vega10:red',


sphinx is not happy about this line… It says:

/home/travis/build/matplotlib/matplotlib/doc/users/colors.rst:22: WARNING: Inline literal start-string without end-string.

I think it is upset that the literal is on several line, but we don't want a block… We'll need to find a solution.

I think just do not put the line break in?

…for vega10 dictionary description in colors.rst

NelleV · 2016-10-29T17:18:59Z

doc/users/colors.rst

-* one of ``{'vega10:blue', 'vega10:orange', 'vega10:green', 'vega10:red',
-            'vega10:purple', 'vega10:brown', 'vega10:pink', 'vega10:gray',
-            'vega10:olive', 'vega10:cyan'}``
+* one of ``{'vega10:blue', 'vega10:orange', 'vega10:green',


NelleV · 2016-10-29T17:19:18Z

There is still a test missing. Apart from that, it looks good!

QuLogic

I'm still against the 10 in the prefix, and I don't think it was properly decided in #7248.

Still waiting for @tacaswell to finish conversation with Tableau and UW, so I'm not really asking for changes, but just blocking on merge (not that this is really binding) until that's resolved.

tacaswell · 2016-10-31T03:16:19Z

This PR should not be held on the Tableau/UW discussion.

There probably does need to be some 10 in the name as we are using 'category10', there is also 'category20' (which is category 10 + lightened variants) and 'category20b' and 'category20c'. The later two have colors that you would want to use the 'simple' names for and will collide with 'c10' / 'c20'.

how about 'vc10' for this namespace? That might be too obscure.

NelleV · 2016-10-31T03:24:35Z

I must be missing something… What's the link between Tableau and D3 (/vega)?

QuLogic · 2016-10-31T03:25:08Z

Yes, category20 is just the light version of category10; that's why I don't think you need the 10. Category10 is the default, thus it is the most likely to be used (except perhaps for non-prefixed colours.) Why add these redundant two characters to every single usage? I'm talking about overall impact to whatever insert-large-number-here of lines is written by the entire userbase, not just the small effect it has on the setup here.

For category20b & c, @anntzer suggested, e.g., blue1, blue2, blue3, ,blue4, because they're all variants of the same colour. But anyway, this is why we should not split the discussion between these two tickets.

NelleV · 2016-10-31T15:58:05Z

I am fine with either, but we need to be entirely sure that there is no possible overlap with the names.

NelleV · 2016-11-01T18:12:31Z

I think we should not merge this for 2.0 but keep in for 2.1.
The naming is not that easy, and it's going to take some time to pin it down correctly. We should not rush it for 2.0.

…o 'vega:<name>'

Elements are fixed

NelleV · 2016-11-08T18:59:13Z

This looks good to me.
@tacaswell should this go in 2.1 or 2.0?

anntzer · 2016-11-10T21:50:27Z

I strongly favor 2.0 from a usability POV.

Updates have been made.

QuLogic · 2016-11-10T22:43:03Z

The only comment I have is there should be a 'grey' spelling as well.

sirlittle · 2016-12-16T18:36:28Z

lib/matplotlib/_color_data.py

+
+# Normalize name to "vega10:<name>" to avoid name collisions.
+VEGA_COLORS = {'vega:' + name: value for name, value in VEGA_COLORS.items()}
+


Maybe change this dictionary comprehension to a for loop for better readability? It seems like too much is being done in one statement.

As far as comprehensions go, this one is pretty simple; I don't think it needs to be rewritten. At most, add a break before for.

tacaswell · 2016-12-16T20:21:13Z

I will come back to this tonight or this weekend.

anntzer · 2017-01-08T05:20:33Z

lib/matplotlib/_color_data.py

+    'cyan': '#17becf'}
+
+
+# Normalize name to "vega10:<name>" to avoid name collisions.


The comment is wrong, it's "vega:".

(... didn't notice this PR has been superseded by #7639...)

tacaswell · 2017-01-10T16:26:52Z

Closing this to reduce confusion.

Adding names to the color in the new Vega10 color cycle

f819a44

NelleV added the Machine Shop label Oct 25, 2016

NelleV changed the title ~~Adding names to the color in the new Vega10 color cycle~~ [WIP] Adding names to the color in the new Vega10 color cycle Oct 25, 2016

NelleV previously requested changes Oct 26, 2016

View reviewed changes

phobson reviewed Oct 26, 2016

View reviewed changes

trpham added 2 commits October 26, 2016 11:15

Add prefix vega10:__ to vega10 colors descriptions in colors.rst, fix…

9d6247f

… pep8 style for VEGA10_COLORS dictionary in _color_data.py

Fix pep8 style for vega10 colors descriptions in colors.rst

56eed35

NelleV reviewed Oct 26, 2016

View reviewed changes

trpham added 2 commits October 26, 2016 11:43

Change prefix for Vega10 colors from 'vega:_' to 'vega10:_'

ded8d66

Change prefix for Vega10 colors from 'vega:<name>' to 'vega10:<name>'…

a270ac8

… in colors.py

tacaswell added this to the 2.0 (style change major release) milestone Oct 26, 2016

NelleV reviewed Oct 27, 2016

View reviewed changes

Fix sphinx warning: 'Inline literal start-string without end-string' …

8950350

…for vega10 dictionary description in colors.rst

NelleV reviewed Oct 29, 2016

View reviewed changes

QuLogic previously requested changes Oct 29, 2016

View reviewed changes

NelleV modified the milestones: 2.0.1 (next bug fix release), 2.0 (style change major release) Oct 31, 2016

NelleV modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Nov 1, 2016

trpham added 2 commits November 1, 2016 11:21

Add test case for Vega10 color

5ae9cba

Change vega10 color package naming convention from: 'vega10:<name>' t…

8b3b971

…o 'vega:<name>'

NelleV changed the title ~~[WIP] Adding names to the color in the new Vega10 color cycle~~ [MRG+1] Adding names to the color in the new Vega10 color cycle Nov 8, 2016

NelleV added the status: needs review label Nov 9, 2016

sirlittle reviewed Dec 16, 2016

View reviewed changes

NelleV removed the Machine Shop label Dec 19, 2016

anntzer requested changes Jan 8, 2017

View reviewed changes

tacaswell modified the milestones: 2.0 (style change major release), 2.1 (next point release) Jan 10, 2017

tacaswell closed this Jan 10, 2017

QuLogic removed the status: needs review label Jan 19, 2017

QuLogic changed the title ~~[MRG+1] Adding names to the color in the new Vega10 color cycle~~ Adding names to the color in the new Vega10 color cycle Jan 19, 2017



		# Normalize name to "vega:<name>" to avoid name collisions.
		VEGA10_COLORS = {'vega:' + name: value for name, value in VEGA10_COLORS.items()}

		'cyan': '#17becf'}


		# Normalize name to "vega:<name>" to avoid name collisions.


		# Normalize name to "vega10:<name>" to avoid name collisions.
		VEGA_COLORS = {'vega:' + name: value for name, value in VEGA_COLORS.items()}

Uh oh!

Conversation

trpham commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NelleV commented Oct 25, 2016

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!

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!

NelleV commented Oct 29, 2016

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Oct 31, 2016

Uh oh!

NelleV commented Oct 31, 2016

Uh oh!

QuLogic commented Oct 31, 2016

Uh oh!

NelleV commented Oct 31, 2016

Uh oh!

NelleV commented Nov 1, 2016

Uh oh!

NelleV commented Nov 8, 2016

Uh oh!

anntzer commented Nov 10, 2016

Uh oh!

QuLogic commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

trpham commented Oct 25, 2016 •

edited

Loading