New color conversion machinery. #6382

anntzer · May 8, 2016

The main goal is to support #rrggbbaa hex color spec (#5461, #6196). The API is also considerably simplified: the conversion API is now reduced to

is_color_like(c)
to_rgba(c, alpha=None)
to_rgba_array(c, alpha=None)
to_hex(c, alpha=None)

Full backwards compatibility is maintained (perhaps the compatibility layer should be deprecated in a later PR).

efiring · May 8, 2016

Based on an initial scan of the code, I like it. It's much clearer and cleaner than the original.

anntzer · May 8, 2016

Can someone help with the test failures on python-2.7/numpy-1.6? I have never been able to locally get 100% matching images in the tests even on recent pythons and numpys so that'll be hard for me to investigate.
It would be nice to get the docs to build too because they also provide quite a bit of testing, but again I can't do much with the given error.

efiring · May 8, 2016

The 2.7 test failures that have just now started appearing are all in the svg images, not png. Either something has changed in Travis, or there is a bad 2.7-specific interaction between the most recent commits and the svg backend.

anntzer · May 8, 2016

Perhaps it would be nice to add Python (most recent) / numpy 1.6 and Python 2.7 / numpy (most recent) to the test matrix to simplify the debugging here.

Also, according to AppVeyor this passes with Python 2.7 / numpy 1.8...

anntzer · May 8, 2016

Got it, this was due to a change in the behavior of the builtin round between Python2 and Python3 (Py2 rounds away from zero, Py3 rounds to even). np.round always rounds to even so use it in to_hex. Tests now pass.

seaborn messes up with the color mapping (for good reasons IMO, and it was part of the public API anyways). I guess this means we should maintain it as public, and hide away the cache. Instead, the color mapping should detect changes to itself and reset the cache as needed.

efiring · May 8, 2016

Regarding the docs build, it seems that the problem occurs as Travis is trying to download and install basemap from github. It has been failing more often than not today--but not always.

jenshnielsen · May 8, 2016

For the docs issue see #6379

efiring · May 8, 2016

@anntzer, your new version of to_rgba_array is nice and compact, but I think it is very inefficient for one important case where we want it to be fast: when its argument is already a valid rgba array, and potentially a large one.

anntzer · May 8, 2016

The latest commit should cover the performance issues.

efiring · May 8, 2016

Another odd 2.7-only failure: in test_boxplot.

anntzer · May 8, 2016

Apparently returning builtin floats instead of numpy floats solves the issue...

I still like the idea of using get_nth_color as an API here. We may change the exact formatting and specifics of this in the future. (Plus, why parse the string twice?)

The previous version may look format independent, but if you check the parsing code just around it it's clearly also dependent on the exact format of CN colors. Plus, it uses a private API. (And I wouldn't worry too much about the performance implications of doing an extra regexp match.)

This seems like a wash either way. The elif statement above is encoding the details of the formatting. Using _get_nth_color only buries half the body.

On the other hand, it is our policy that we can use mpl private API anywhere in the mpl code base (that is the point of the private APIs).

I tend to think that private APIs are module-level (or class-level) private (rather than project-level), but don't feel strongly about this so either way works for me.

mdboom · May 9, 2016

Thanks for this much needed improvement. I have nothing to add to @efiring's comments.

anntzer · May 9, 2016

Moved 1-letter codes to their own mapping in _color_data.
Do we want to deprecate the old API in code too? IMO the main reason to do so is not because it is ugly :-), but because it favors silently dropping alpha (to_rgb and rgb2hex).

tacaswell · May 9, 2016

I am 👎 on dropping the old API.

It is a very old part of the public API so may have wide-spread use in user code and it is not onerous to continue supporting.

anntzer · May 9, 2016

Fair enough, but what about at least removing it from the docs (via sphinx' autodoc-skip-member), i.e. deprecate-in-docs? By the way, the current API entry is rather ugly: http://matplotlib.org/api/colors_api.html#matplotlib.colors.ColorConverter

tacaswell · May 9, 2016

I am fine with removing it from the docs / re-writing those docs.

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

Fair enough, but what about at least removing it from the docs (via
sphinx' autodoc-skip-member), i.e. deprecate-in-docs? By the way, the
current API entry is rather ugly:
http://matplotlib.org/api/colors_api.html#matplotlib.colors.ColorConverter

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#6382 (comment)

anntzer · May 9, 2016

Now that we're there, what about replacing the "do-nothing" plt.colors function (only there for its docstring) by the matplotlib.colors module itself? The module docstring can be rewritten in a way suitable for interactive doc reading too.

anntzer · May 9, 2016

Also, I just noticed that there are collisions between css4 color names and xkcd color names (e.g., "yellow") -- this was why the previous commit broke the tests, as I overwrote the css4 colors with the xkcd colors instead of the other way round (now fixed). So I'm going to make another impopular (aka. backwards incompatible) proposal: rename all xkcd colors to "xkcd:", and remove the other aliases (it's not as if "xkcdpastel red" looked particularly nice either).

tacaswell · May 9, 2016

We have not released the xckd colors yet so we can still break that.

I think that should get its own issue separate from this (as that should be
back ported where as this should not be).

On Mon, May 9, 2016, 12:52 Antony Lee notifications@github.com wrote:

Also, I just noticed that there are collisions between css4 color names
and xkcd color names (e.g., "yellow") -- this was why the previous commit
broke the tests, as I overwrote the css4 colors with the xkcd colors
instead of the other way round (now fixed). So I'm going to make another
impopular (aka. backwards incompatible) proposal: rename all xkcd colors to
"xkcd:", and remove the other aliases (it's not as if "xkcdpastel red"
looked particularly nice either).

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#6382 (comment)

anntzer · May 10, 2016

I am trying to work on the docs update but ./make.py html immediately runs into

Running Sphinx v1.4.1

Warning, treated as error:
WARNING: while setting up extension ['?', 'matplotlib.sphinxext.mathmpl']: node class 'latex_math' is already registered, its visitors will be overridden

Building HTML failed.

which I have seen on Travis sometimes too.
Any hints?

tacaswell · May 10, 2016

This should have been addressed by #6238

Have we not done the merge cascade yet?

anntzer · May 10, 2016

Rebasing on master did not help.

tacaswell · May 11, 2016

Try downgrading to sphinx 1.3.x?

Add xkcd here or does that just blow the example up too much?

Try making a figure with 3244 lines and text elements and get back to me :-)

fair point.

This seems like a reasonable place to use to_rgb

Does GTK have the concept of an RGBA color? (Not according to http://www.pygtk.org/pygtk2reference/class-gdkcolor.html, but I don't known much about GTK...)
Frankly I'd rather not expose to_rgb as a public API anymore, it's too easy to lose alpha information on the way...

@fariza is the GTK expert.

There are some applications which do not know / use / care about alpha, forcing all of them to explicitly drop the last value is falling a bit too far to the 'purity' side of API design.

OK, I have restored to_rgb, but will wait until we get a response re. GTK to decide what to do here (and won't push for now).

This is a section of the code that I was waiting for MEP27 to get rid of it. I plan to introduce the same functionality as generic tool for all the backends.

This class DialogLineprops is used only by one example: examples/user_interfaces/lineprops_dialog_gtk.py

The class was copy/paste to Gtk3 but last time I checked it wasn't working.

My advice would be, if the example runs, then it is fine. (I don't have a working gtk2 environment anymore to test)

The example runs with py2/gtk2. However, it is the typical example of why I want to get rid of to_rgb: the line props dialog seemingly support setting alpha (the glade file explicitly sets use_alpha to True), but the alpha is silently dropped after.

~~Where does the cache get used?~~

The cache is needed for performance purposes. In its absence, matplotlib.tests.test_lines.test_invisible_Line_rendering fails.

seaborn needs to reset the cache when changing the named colors mapping (https://github.com/mwaskom/seaborn/blob/dfdd1126626f7ed0fe3737528edecb71346e9eb0/seaborn/palettes.py#L953), most likely because changing just the main mapping would have no effect on already cached conversions. So I need to maintain it as "semi-public" (via colorConverter), but I'd rather make it an implementation detail (thus the _ColorMapping class instead).

Given how this is used, it might make more sense to make cache a public attribute.

seaborn probably should not do that, but that is a different discussion. At least it is limited to the single letter colors.

That use-case should be eliminated by a combination of setting the color cycle + the 'CN' color specification.

This needs a cut-out incase the default prop_cycle does not contain a color key. The default in this case should probably be 'k'

This was copypasted from the previous implementation. I don't know anything about cycler, but I guess prop_cycler._transpose().get('color', 'k') would work?

This is looking up based on a tuple with a float in it, that might lead to a ballooning of the cache size?

I want to suggest just caching the rgb values, but keep convincing my self every suggestion I type out will not work.

It is obviously possible to make an alpha-less cache work, but I'm not sure the memory savings are worth it. On the other hand, that would certainly be a precondition for making the cache public (otherwise it is truly too implementation-specific and should be kept private).

tacaswell · May 11, 2016

I like this 👍

I have wanted to get rid of that singleton for a while, but never got around to doing it.

anntzer · May 11, 2016

"this" being?

tacaswell · May 11, 2016

the whole PR 😄

anntzer · May 11, 2016

Sorry, I meant "that" (singleton).
Thanks :p

this singleton

but you won't let me deprecate it :-(

Do you want to deal with breaking seaborn 😈

I didn't say remove, just deprecate...

tacaswell · May 11, 2016

And I am suggesting making cache a public attribute on the private dictionary sub-class / private instance of said sub class, not making the cache part of the public API.

anntzer · May 11, 2016

I feel like the main remaining question is whether we (mostly meaning you) want to draw an official policy w.r.t. changing the named color mapping. I agree that long names should not be modified, but I do like the remapping of one-letter codes by seaborn (because it makes making a plot with a consistent color style much easier).

anntzer · May 11, 2016

Rebased on master, restored to_rgb, removed the old converters from the docs (Note the change to conf.py, it may be reused for others to remove more obsolete APIs from the docs.).

tacaswell · May 12, 2016

Can you also add a whats_new documenting the new API?

Did you ever get the docs to build locally?

tacaswell · May 12, 2016

This needs a whats_new entry.

Can you add a test where round-tripping a #AABBCCFF hex code? The coverage report suggests that that code path is not tested.

Other wise I am 👍 on merging this.

anntzer · May 12, 2016

Actually I just realized one issue: should to_hex(color) convert RGB triplets (with no alpha set, and when called without the alpha kwarg) to #rrggbb? Currently it returns #rrggbbFF (alpha=255) but a case could actually be made to return #rrggbb (which is after all a standard) only in this case. Thoughts?

I did get the docs built after git clean -xfd, which is unsuprisingly the solution to all ills...

efiring · May 12, 2016

Yes, I don't see any benefit in tacking on "FF" willy-nilly. It's just noise. And it's not really part of the color. Alpha modifies the way colors are rendered when they overlap--it is not itself part of a color specification.

anntzer · May 12, 2016

OK, let's think of a better API (I do want to keep a way to convert to #rrggbbaa, this is useful e.g. for the Qt options editor and more generally "readable" text serialization). Something basic would be

to_hex(color, alpha=None, keep_alpha=False):

which would drop alpha unless keep_alpha is set, but the redundancy between the two kwargs is a bit awkward.

tacaswell · May 12, 2016

Maybe drop the alpha kw on to_hex? If you want to inject alpha you can always do: to_hex(to_rbga(c, alpha=alpha), keep_alpha=True)

anntzer · May 12, 2016

Good suggestion, implemented.
Also pushed updated the qt editor alpha PR to use the new API.

tacaswell · May 12, 2016

Sorry, I think I induced this merge by merging the xkcd rename branch 😞

anntzer · May 12, 2016

...no? they're independent. (The Qt editor alpha depends on this one though.)

tacaswell · May 14, 2016

This still needs a rebase

anntzer · May 14, 2016

Done.

tacaswell · May 14, 2016

@anntzer Thanks for this and thank you for putting up with our slow code review.

anntzer · May 14, 2016

No worries, thanks. Also updated #6383 accordingly.

QuLogic · Jun 15, 2016

Backport to v2.x is 3281bcd.

mdboom added the needs_review label May 8, 2016

anntzer referenced this pull request May 8, 2016
Merged
Qt editor alpha #6383

tacaswell added this to the 2.1 (next point release) milestone May 8, 2016

tacaswell closed this May 8, 2016

tacaswell reopened this May 8, 2016

tacaswell added needs_review and removed needs_review labels May 8, 2016

anntzer referenced this pull request May 9, 2016
Merged
Use xkcd: prefix to avoid color name clashes. #6390

tacaswell removed the needs_review label May 14, 2016

anntzer deleted the anntzer:rgba-support branch May 14, 2016

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 16, 2016

tacaswell Merge pull request #6382 from anntzer/rgba-support
ENH/API: New color conversion machinery
3281bcd

efiring added a commit that referenced this pull request May 16, 2016

efiring Merge pull request #6435 from tacaswell/backport_rgba_overhual
Merge pull request #6382 from anntzer/rgba-support
3b8a6ec

This was referenced May 19, 2016

Closed

Feature request: allow a default line alpha to be set in mpl.rcParams #5461

Closed

ENH: Set the alpha value for plots in rcParams #5132

Closed

Using to_rgba rather than to_rgb in figure_edit #6589

QuLogic modified the milestone: 2.0 (style change major release), 2.1 (next point release) Jun 15, 2016

This was referenced Jun 28, 2016

Merged

Implement plotting using matplotlib Collections geopandas/geopandas#267

Merged

Don't convert vmin, vmax to floats. #6700

Merged

Minor tab colour fix #7847

                  if color is not None:
                      raise ValueError(
                          'Illegal format string "%s"; two color symbols' % fmt)
                  color = c
              elif c == 'C' and i < len(chars) - 1:
                  color_cycle_number = int(chars[i + 1])
-    -            color = mcolors.colorConverter._get_nth_color(color_cycle_number)
+    +            color = mcolors.to_rgba("C{}".format(color_cycle_number))

-    -
-    -# Transform to hex color values.
-    -hex_ = [color[1] for color in colors_]
-    -# Get the rgb equivalent.
-    -rgb = [colors.hex2color(color) for color in hex_]
-    -# Get the hsv equivalent.
-    -hsv = [colors.rgb_to_hsv(color) for color in rgb]
-    -
-    -# Split the hsv values to sort.
-    -hue = [color[0] for color in hsv]
-    -sat = [color[1] for color in hsv]
-    -val = [color[2] for color in hsv]
-    -
-    -# Get the color names by themselves.
-    -names = [color[0] for color in colors_]
+    +colors = dict(mcolors.BASE_COLORS, **mcolors.CSS4_COLORS)

@@ -1003,13 +999,13 @@ def on_combobox_lineprops_changed(self, item):
              if marker is None: marker = 'None'
              self.cbox_markers.set_active(self.markerd[marker])
-    -        r,g,b = colorConverter.to_rgb(line.get_color())
-    -        color = gtk.gdk.Color(*[int(val*65535) for val in (r,g,b)])
+    +        rgba = mcolors.to_rgba(line.get_color())

-    -COLOR_NAMES = {'xkcd': XKCD_COLORS,
-    -               'css4': CSS4_COLORS}
+    +class _ColorMapping(dict):
+    +    def __init__(self, mapping):
+    +        super(_ColorMapping, self).__init__(mapping)
+    +        self._cache = {}

+    +    If `alpha` is not `None`, it forces the alpha value.
+    +    """
+    +    # Special-case nth color syntax because it should not be cached.
+    +    if _is_nth_color(c):
+    +        from matplotlib import rcParams
+    +        from matplotlib.rcsetup import cycler
+    +        prop_cycler = rcParams['axes.prop_cycle']
+    +        if prop_cycler is None and 'axes.color_cycle' in rcParams:
+    +            clist = rcParams['axes.color_cycle']
+    +            prop_cycler = cycler('color', clist)
+    +        colors = prop_cycler._transpose()['color']

matplotlib/matplotlib

Join GitHub today

New color conversion machinery. #6382

Conversation

anntzer commented May 8, 2016

mdboom added the needs_review label May 8, 2016

anntzer referenced this pull request May 8, 2016

efiring commented May 8, 2016

anntzer commented May 8, 2016

efiring commented May 8, 2016

anntzer commented May 8, 2016 • edited

anntzer commented May 8, 2016 • edited

efiring commented May 8, 2016

jenshnielsen commented May 8, 2016

efiring commented May 8, 2016

tacaswell added this to the 2.1 (next point release) milestone May 8, 2016

tacaswell closed this May 8, 2016

tacaswell reopened this May 8, 2016

tacaswell added needs_review and removed needs_review labels May 8, 2016

anntzer commented May 8, 2016

efiring commented May 8, 2016

anntzer commented May 8, 2016

mdboom commented on the diff May 9, 2016

mdboom commented May 9, 2016

anntzer commented May 9, 2016

tacaswell commented May 9, 2016

anntzer commented May 9, 2016

tacaswell commented May 9, 2016

anntzer commented May 9, 2016

anntzer commented May 9, 2016

tacaswell commented May 9, 2016

anntzer referenced this pull request May 9, 2016

anntzer commented May 10, 2016

tacaswell commented May 10, 2016

anntzer commented May 10, 2016

tacaswell commented May 11, 2016

tacaswell commented on the diff May 11, 2016

tacaswell commented on the diff May 11, 2016

anntzer May 11, 2016 • edited

Show 4 comments Hide comments tacaswell and 1 other commented on an outdated diff May 11, 2016

tacaswell May 11, 2016 • edited

Show 2 comments Hide comments tacaswell and 1 other commented on an outdated diff May 11, 2016

Show 2 comments Hide comments tacaswell and 1 other commented on an outdated diff May 11, 2016

tacaswell commented May 11, 2016

anntzer commented May 11, 2016

tacaswell commented May 11, 2016

anntzer commented May 11, 2016 • edited

tacaswell commented on the diff May 11, 2016

tacaswell commented May 11, 2016

anntzer commented May 11, 2016

anntzer commented May 11, 2016

tacaswell commented May 12, 2016

tacaswell commented May 12, 2016

anntzer commented May 12, 2016

efiring commented May 12, 2016

anntzer commented May 12, 2016

tacaswell commented May 12, 2016

anntzer commented May 12, 2016

tacaswell commented May 12, 2016

anntzer commented May 12, 2016

tacaswell commented May 14, 2016

anntzer added some commits May 6, 2016

anntzer commented May 14, 2016

anntzer added some commits May 8, 2016

Some checks were not successful

Hide details View details tacaswell merged commit 22a7b95 into matplotlib:master May 14, 2016 1 of 3 checks passed

1 of 3 checks passed

tacaswell removed the needs_review label May 14, 2016

tacaswell commented May 14, 2016

anntzer deleted the anntzer:rgba-support branch May 14, 2016

anntzer commented May 14, 2016

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 16, 2016

efiring added a commit that referenced this pull request May 16, 2016

This was referenced May 19, 2016

QuLogic commented Jun 15, 2016

QuLogic modified the milestone: 2.0 (style change major release), 2.1 (next point release) Jun 15, 2016

This was referenced Jun 28, 2016

tacaswell added a commit to QuLogic/matplotlib that referenced this pull request Jan 16, 2017

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

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

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

tacaswell merged commit `22a7b95` into matplotlib:master May 14, 2016
1 of 3 checks passed