Support Cn colors with n>=10. #12598

anntzer · 2018-10-23T12:28:51Z

While the change is mildly backwards-incompatible (as noted in the API
changes), Cn syntax is by far the simplest way to navigate the
color-cycle, so I think the change is worth it.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

story645 · 2018-10-23T15:24:34Z

doc/api/next_api_changes/2018-10-23-AL.rst

+``Cn`` syntax, e.g. ``plt.plot([1, 2], color="C11")`` now uses the 11th color
+in the cycle.
+
+Note that previously, a construct such as ``plt.plot([1, 2], "C11")`` would be


Are there any gallary examples that need to be updated to show this?

A quick grep doesn't yield anything relevant.

ImportanceOfBeingErnest · 2018-10-23T16:37:36Z

doc/api/next_api_changes/2018-10-23-AL.rst

+```````````````````````````````````
+
+It is now possible to go beyond the tenth color in the property cycle using
+``Cn`` syntax, e.g. ``plt.plot([1, 2], color="C11")`` now uses the 11th color


C11 should be the 12th color, right?

~~In general what happens if you specify C10 on a cycler with 10 colors? Is it ensured that it will result in the first color then?~~ Sorry that is indeed tested for.

Depends on whether you think it starts at zeroth or first. I think matching the idex is easier, if not necessarily correct in English.
In any case I think the meaning is clear...

Ähm???? Sorry but a[1] takes the second element of a, not the first. That has nothing to do with language.

Sure, changed.

ImportanceOfBeingErnest · 2018-10-23T17:09:39Z

I would agree that the described API change in the color-marker-whatever-string-minilanguage ("C11"->"1C1") should not block this.

However further documentation changes are necessary:

This will require to remove the words "single digit" from

I think this format string language is documented only in the documentatio of plot. Currently it says:

A format string consists of a part for color, marker and line:

    fmt = '[color][marker][line]'

So this should be updated to allow for fmt = '[marker][color][line]'. But is this true in general?

Also the docs currently say

The following color abbreviations are supported:
'b' | blue
'g' | green
'r' | red
'c' | cyan
'm' | magenta
'y' | yellow
'k' | black
'w' | white
If the color is the only part of the format string, you can additionally use any matplotlib.colors spec, e.g. full names ('green') or hex strings ('#8000').

Here the Cn notation isn't even mentionned. So technically, "C11" has never been officially supported?

anntzer · 2018-10-23T17:45:08Z

Fixed the docs (moving the color section of plot below, to keep them in order).
Cn specs may not have been officially supported in the minilanguage format, but given that there's code explicitly to handle that in the minilanguage parser, it's really just a doc oversight.

ImportanceOfBeingErnest

This is overall a pretty useful extension. Since it turns out that the format string language hasn't been specified as detailed as to actually support the Cn notation at all and the only breaking case is for digit markers, which are not very popular anyways, this seems like a change that can be made without deprecation.

tacaswell · 2018-10-24T10:30:37Z

I am +.5 on this. I see the logic and motivation, but it is now treating strings that used to be equivalent differently.

anntzer · 2018-10-24T11:37:06Z

You mean the API changes note, or something else?

tacaswell · 2018-10-24T12:28:16Z

I mean the trade off of if the improved functionality worth breaking some (hopefully small number) of users.

timhoffm · 2018-10-24T19:45:33Z

lib/matplotlib/axes/_axes.py

@@ -1517,33 +1517,12 @@ def plot(self, *args, scalex=True, scaley=True, **kwargs):

        A format string consists of a part for color, marker and line::

-            fmt = '[color][marker][line]'
+            fmt = '[marker][line][color]'


Not sure it is worth to make that change. Anyway we should explicitly state that order does not matter.

Well, it does in the sense that C11 doesn't mean the same thing as 1C1.

I feel that 'b-' is much more used in practical code than '-b'. Maybe also because of the current docs. I wouldn't want to change the habits of users for that. 'C11' really an edge case, and is just now eagerly collecting digits. People can still go for '1C1' in that specific case (or use kwargs), I would not generally recommend switching the order.

I'm not recommanding it (or even coming closing to thinking about deprecating the other order); I'm just putting the docs in the order that always works...
At least if by any chance a new user discovers the minilanguage by reading this specific docstring (which likely never happens), they'll have it in the right order.

Two options:

Leave fmt = '[color][marker][line] as before. Add a sentence about the potential problem, like

Note that this may lead to an ambiguity when using single digit markers ("1") in combination with the "Cn" color notation, i.e. "C11" means the twelfth color without marker. To provide the option to use the 1 marker and the second color, use "1C1".

Keep the rest of the docstring the same.

Change it to fmt = '[marker][line][color]'. In that case add a sentence like,

Note that other combinations like [color][marker][line] are equally supported but might lead to ambiguities concerning colors and markers.

Then change the example strings further down the page to match with the new format.

add a sentence like,

Note that other combinations like [color][marker][line] are equally supported but might lead to ambiguities concerning colors and markers.

Done.

Then change the example strings further down the page to match with the new format.

That was already done.

timhoffm · 2018-10-24T19:50:13Z

lib/matplotlib/tests/test_rcparams.py

@@ -307,7 +307,7 @@ def generate_validator_testcases(valid):
                     ('AABBCC', '#AABBCC'),  # RGB hex code
                     ('AABBCC00', '#AABBCC00'),  # RGBA hex code
                     ('tab:blue', 'tab:blue'),  # named color
-                     ('C0', 'C0'),  # color from cycle
+                     ('C123', 'C123'),  # color from cycle


Would just (or additionally) test "C12", which is a pontential ambiguous definition, which is more likely to break when changing the parser code.

timhoffm · 2018-10-24T19:57:04Z

+0.5 on this. It's a breaking change, but for a more sane API. I assume that there have been more mistakes made with "C11" intending to get the 11th color, than there have been people, who really wanted to get "C1" with marker "1". Even if I really wanted that, I would have used explicit kwargs, because using "C11" with the marker "1" semantics feels like dirty code.

anntzer · 2018-10-24T23:48:07Z

If we're really worried about the API break I can probably make "C11" (as an arg, not a kwarg) still keep its old meaning; at least prople will now be able to use color="C11" which is unambiguous.
However I think this is would really qualify as backcompat for backcompat's sake and mostly just leave a way to trip users, rather than being helpful.

tacaswell · 2018-10-25T01:58:11Z

However I think this is would really qualify as backcompat for backcompat's sake and mostly just leave a way to trip users, rather than being helpful.

I agree, I think that would be worse than breaking 'C11' vs '1C1' equivalence.

jklymak

This seems fine. The obscure API break is pretty obscure and easily worked around.

jklymak · 2018-10-26T19:42:17Z

@anntzer is this ready to merge, or were you still playing w. it?

anntzer · 2018-10-28T14:00:39Z

@tacaswell asked @WeatherGod to review this, let's give him a few more days to chime in.

jklymak · 2018-10-29T20:01:10Z

ping @WeatherGod - we'll merge if we don't hear from you by, say, Friday?

WeatherGod

I am +0 on this. I really don't see the point of needing this many colors in your graph, and if you did need that many colors, to have a need to access those upper colors through a mechanism that was intended for simple scripts and interactive uses. If you are plotting that many things to need that many colors, I sure hope you have some loops in your code, in which case, it isn't that difficult to get the colors yourself.

The fact that this introduces a slight change in interpretation is concerning, but not the end-of-the-world. CN notation has not been around for very long, and '1C1' is more readable than 'C11'.

We need to do something about the documentation stating that the color is indexed at artist creation time. That may have been the original intent, but it certainly isn't always the case now, and I think "at the time of rendering" makes more sense as a target goal. At the very least, we should throw out this statement and leave it ambiguous until we solve the issue across the board.

WeatherGod · 2018-10-31T00:44:03Z

examples/color/color_demo.py

-   creation time and defaults to black if the cycle does not include color.
+7) a "Cn" color spec, i.e. `'C'` followed by a number, which is an index into
+   the default property cycle (``matplotlib.rcParams['axes.prop_cycle']``); the
+   indexing occurs at artist creation time and defaults to black if the cycle


while we are here, we should fix this statement "at artist creation time" and make it "at the time of rendering". I know we aren't quite there yet, but this current statement isn't true, either.

We should also mention that this is a goal and we‘re not yet there. Having an uncommitted false statement in the docs is misleading.

WeatherGod · 2018-10-31T00:49:30Z

lib/matplotlib/colors.py

-  cycle does not include color.
+* a "CN" color spec, i.e. `'C'` followed by a number, which is an index into
+  the default property cycle (``matplotlib.rcParams['axes.prop_cycle']``); the
+  indexing occurs at artist creation time and defaults to black if the cycle


ditto here. "artist creation time" --> "at the time of rendering"

WeatherGod · 2018-10-31T00:52:26Z

tutorials/colors/colors.py

-  cycle does not include color.
+* a "CN" color spec, i.e. `'C'` followed by a number, which is an index into
+  the default property cycle (``matplotlib.rcParams['axes.prop_cycle']``); the
+  indexing occurs at artist creation time and defaults to black if the cycle


ditto here, too.

anntzer · 2018-10-31T09:37:31Z

I am +0 on this. I really don't see the point of needing this many colors in your graph, and if you did need that many colors, to have a need to access those upper colors through a mechanism that was intended for simple scripts and interactive uses. If you are plotting that many things to need that many colors, I sure hope you have some loops in your code, in which case, it isn't that difficult to get the colors yourself.

Actually another use case (which motivated this PR to start with) is to rely on cycling, e.g. having only 10 colors in the cycle but wanting to plot 20 lines, then you can just color them with C0...C19 without bothering of doing things modulo... well you can't even do it modulo len(prop_cycle) because 1) I think there isn't a getter on the prop cycle? and more importantly 2) the prop cycle that matters is the one at rendering time, which we don't know yet.

While the change is mildly backwards-incompatible (as noted in the API changes), Cn syntax is by far the simplest way to navigate the color-cycle, so I think the change is worth it.

anntzer · 2018-10-31T13:17:03Z

Updated the docs with "... is intended to occur at rendering time" given that the current behavior is a mess... (would be nice to reach a decision wrt #11711, btw)

Yes, the wording is much better. I am still +0 on this, so I will dismiss my request for changes to unblock this.

anntzer added this to the v3.1 milestone Oct 23, 2018

anntzer force-pushed the cn branch from b5eba12 to 53d828f Compare October 23, 2018 13:16

story645 reviewed Oct 23, 2018

View reviewed changes

ImportanceOfBeingErnest reviewed Oct 23, 2018

View reviewed changes

anntzer force-pushed the cn branch from 53d828f to 2d02729 Compare October 23, 2018 17:44

anntzer force-pushed the cn branch from 2d02729 to e1876a2 Compare October 23, 2018 18:12

ImportanceOfBeingErnest approved these changes Oct 23, 2018

View reviewed changes

tacaswell requested a review from WeatherGod October 23, 2018 22:59

timhoffm reviewed Oct 24, 2018

View reviewed changes

anntzer force-pushed the cn branch from e1876a2 to 05ce6c5 Compare October 24, 2018 23:45

tacaswell added the API: changes label Oct 25, 2018

anntzer force-pushed the cn branch from 05ce6c5 to 35a1281 Compare October 25, 2018 14:40

jklymak approved these changes Oct 26, 2018

View reviewed changes

WeatherGod previously requested changes Oct 31, 2018

View reviewed changes

Support Cn colors with n>=10.

aa96f2d

While the change is mildly backwards-incompatible (as noted in the API changes), Cn syntax is by far the simplest way to navigate the color-cycle, so I think the change is worth it.

anntzer force-pushed the cn branch from 35a1281 to aa96f2d Compare October 31, 2018 13:16

jklymak merged commit e2625d0 into matplotlib:master Nov 5, 2018

anntzer deleted the cn branch November 5, 2018 18:52

QuLogic mentioned this pull request Apr 26, 2023

CN colors look up the global prop_cycle at draw time rather than the axes' one. #9842

Closed

Uh oh!

Support Cn colors with n>=10. #12598

Support Cn colors with n>=10. #12598

Uh oh!

Conversation

anntzer commented Oct 23, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ImportanceOfBeingErnest Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ImportanceOfBeingErnest commented Oct 23, 2018

Uh oh!

anntzer commented Oct 23, 2018

Uh oh!

ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Oct 24, 2018

Uh oh!

anntzer commented Oct 24, 2018

Uh oh!

tacaswell commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

timhoffm commented Oct 24, 2018

Uh oh!

anntzer commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Oct 25, 2018

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Oct 26, 2018

Uh oh!

anntzer commented Oct 28, 2018

Uh oh!

jklymak commented Oct 29, 2018

Uh oh!

WeatherGod 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!

ImportanceOfBeingErnest Oct 23, 2018 •

edited

Loading

tacaswell commented Oct 24, 2018 •

edited

Loading

anntzer commented Oct 24, 2018 •

edited

Loading

anntzer commented Oct 31, 2018 •

edited

Loading