New color conversion machinery. #6382
Conversation
|
Based on an initial scan of the code, I like it. It's much clearer and cleaner than the original. |
|
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. |
|
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. |
|
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... |
3cc1575 to
526a0e8
Compare
|
Got it, this was due to a change in the behavior of the builtin 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. |
|
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. |
|
For the docs issue see #6379 |
|
@anntzer, your new version of |
|
The latest commit should cover the performance issues. |
|
Another odd 2.7-only failure: in |
|
Apparently returning builtin floats instead of numpy floats solves the issue... |
| 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)) |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
Thanks for this much needed improvement. I have nothing to add to @efiring's comments. |
|
Moved 1-letter codes to their own mapping in |
|
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. |
|
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 |
|
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:
|
|
Now that we're there, what about replacing the "do-nothing" |
|
@anntzer Thanks for this and thank you for putting up with our slow code review. |
|
No worries, thanks. Also updated #6383 accordingly. |
ENH/API: New color conversion machinery
Merge pull request #6382 from anntzer/rgba-support
|
Backport to v2.x is 3281bcd. |
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
The main goal is to support
#rrggbbaahex color spec (#5461, #6196). The API is also considerably simplified: the conversion API is now reduced tois_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).