That might be an aggressive deprecation schedule, there are a lot of examples of people relying on color cycle in scary ways. I suspect we are going to have to stub out the get/set methods and provide a read/write property to fake up the old way for a very long time (3.0?).

Can't we keep both (at least for now)?
If we only have one we use the one that we have.
If we have both we can first try to merge the cycles using the inner product; if that fails (because the stylecycle already contains a colorcycle) then we use the stylecycle.

Actually, what is getting removed/changed are methods of a class that starts with an underscore and is generated dynamically, and is accessed through methods that have an underscore. I kept around Axes.set_color_cycle(), though.

Ahh, sorry, I didn't see the class name, I dislike that github doesn't show that when it snips.

Just thinking about this a bit. One thing that breaks backwards compatibility is that axes.color_cycle is pretty much completely ignored. While the default prop_cycle is identical to the old approach, what if someone applies a style sheet that does not have this new cycle, or if someone programmatically sets this rc param.

Maybe it makes sense to augment the axes.color_cycle semantics rather than adding a new property? So, if someone passes a simple list of colors, then transparently create a cycler object for colors. But if it is a cycler object (or a string that can be eval'ed as a cycler), then we treat it as such?

I think we can fake complete back compatibility with enough @properties.

Just pushed up some more fixes that should bring down the number of test failures/errors significantly.

@tacaswell , I am not sure how you envision using properties to solve the problem I mentioned. The rc params is just a giant dictionary, right? I think we need to have a mechanism for aliasing rcparam values.

Oh, wow... the standard set of tests pass (it doesn't on my system due to font issues). Ok, I'll put up some tests tonight and see how they do.

I am still second-guessing myself with the creation of a new rcparam and deprecation of the current one, as opposed to over-loading the current one.

added some tests, discovered that bar() doesn't support property cycling (but fill() does). Although, I can't seem to get hatching to cycle for it...

hatching + color seems to not get along, try setting 'facecolor' and 'edgecolor' different. I ran into this last night in https://github.com/matplotlib/matplotlib/pull/4433/files?diff=unified

re: hatching, confirmed that using facecolor instead of color allowed the hatching to work. I suspect what is happening is that the hatch is getting the same color as 'color'. If I set 'facecolor' to 'none', then the hatches show up. Shouldn't hatches default color come from 'lines.linecolor'? Do we have a way to set the color of a hatch?

That all sounds like something to fix in 2.0!

@WeatherGod What is the state of this?

I need to figure out how to avoid the color/facecolor collision issue. I
have an idea for a more complete way of dealing with it, but it would be a
major work. I will work on it tonight.

On Thu, Aug 6, 2015 at 4:09 PM, Thomas A Caswell notifications@github.com
wrote:

@WeatherGod https://github.com/WeatherGod What is the state of this?

—
Reply to this email directly or view it on GitHub
#4686 (comment)
.

So, I had an idea last night about how to fix the issue with setting properties. Consider the situation of a "color" and an "edgecolor" being specified for a Polygon object at the same time:

p = Polygon(np.random.rand(N, 2), color='r', edgecolor='b')

Right now, that will result in a warning, and the edgecolor will end up being red (Don't know if that should be desired behavior, but that's a separate issue.). This is because the Patch constructor tries to handle potential conflicts with properties. However, if I subsequently do a p.set() on that object:

p.set(color='yellow', edgecolor='k')

Then the result is random because it is iterating over a dictionary. If you set color after setting edgecolor, then both the face and the edge will be yellow, but if the edgecolor gets set after the color, then the edge will be black while the face will be yellow. Personally, I think that the latter result should be the desired behavior, but for consistency with the constructor, I think the former result should be desired.

So, what is my solution? A more complete solution would be that each artist subclass should be defining a priority order for their properties. That priority order would then be used by Artist.set() and anyplace else in our codebase where we are iterating over properties and setting them. Creating that priority list might be a bit too much to do right now, though. So, I think that it would be sufficient to patch Artist.set() and a few other places in the codebase to just reverse sort the kwarg dictionary keys and process them in that direction. That way, "color" will always be processed after "linecolor", "facecolor", "edgecolor", "lc", "fc", "ec", etc.

At the very least, this should resolve the randomness issue we are encountering, and it should solve the issue we are having with wrapping up this particular PR. Thoughts?

👍 on the sort.

I think the precedence order issues can be addressed by traitlets @rmorshea ?

That said, there is also a question of user intent. If I do

ln.set_facecolor('r')
ln.set_color('g')

I expect everything about the line to be green.

Another option is to have a normalize_colors method in cbook which we can use everywhere we take kwargs and in in set to handle conflicts between the various color spellings (something like new_dict = normalize_colors(kwarg_dict_maybe_with_colors) )

In the example you just gave, the properties are set independently of each
other, and so whatever happens last happens. We have no control over that,
and my suggestion does not impact that. This is only a question of what
should be done when a batch of properties are set via setp() and
artist.set(), and anyplace else where we are manually looping through
keyword arguments. I am right now testing out the idea.

On Fri, Aug 14, 2015 at 3:15 PM, Thomas A Caswell notifications@github.com
wrote:

[image: 👍] on the sort.

I think the precedence order issues can be addressed by traitlets
@rmorshea https://github.com/rmorshea ?

That said, there is also a question of user intent. If I do

ln.set_facecolor('r')
ln.set_color('g')

I expect everything about the line to be green.

Another option is to have a normalize_colors method in cbook which we can
use everywhere we take kwargs and in in set to handle conflicts between
the various color spellings (something like new_dict =
normalize_colors(kwarg_dict_maybe_with_colors) )

—
Reply to this email directly or view it on GitHub
#4686 (comment)
.

@WeatherGod Sorry, my last comment was not well thought and and missing a big chunk of context that was in my head. That context is having set_color look at facecolor and friends to determine if they are non-default and if so to not set them. I publicly convinced my self that was a bad idea without bothering to tell you all what I was convincing my self not to advocate for.

@tacaswell If I correctly understand what you're attempting to do with precedential assignments, there isn't any Traitlets magic that will do that for you out of the box. Ordering the actual value assignments would probably have similar solutions in Traitlets as it would elsewhere (I'd probably use a context manager if I were to do it). On the other hand, ordering the "trait change callbacks" is something that could be done with minimal modifications to hold_trait_notifications in HasTraits, however I don't think would solve anything unless the callback for a change in 'color' simply triggered an assignment to 'edgecolor'.

I think I got it! Wait 'till you see this doozy!

There are still some odds-n-ends to complete: documentation, testing of new rcparams, rebasing off master to get the classic style, adding prop_cycle to classic style, error handling of invalid properties in batch setting.

There is one potential API change. In some areas of the codebase setting an invalid property would result in an AttributeError, while other areas, it raised a TypeError. I right now have it unified as an explicit TypeError, but I could change that back if people think it matters.

@WeatherGod You managed to fail all of the tests without glaring syntax errors!

Part of the problem is a difficulty in keeping track of what a Cycler object really is. I guess it is an iterable that returns a sequence of dictionaries, each of which has the same keys. I'm now at a loss as to what to call the mpl factory function, but "cyl" still seems too obscure to me. "dict_sequence" is descriptive, but long and clunky. "kwdicts"? Still unpronounceable. I don't want to make too much of a fuss, but naming does matter for readability and usability.

I think many of the problems we have API wise are that not enough of a fuss
was made over naming.

On Fri, Aug 21, 2015 at 4:42 PM Eric Firing notifications@github.com
wrote:

Part of the problem is a difficulty in keeping track of what a Cycler
object really is. I guess it is an iterable that returns a sequence of
dictionaries, each of which has the same keys. I'm now at a loss as to what
to call the mpl factory function, but "cyl" still seems too obscure to me.
"dict_sequence" is descriptive, but long and clunky. "kwdicts"? Still
unpronounceable. I don't want to make too much of a fuss, but naming does
matter for readability and usability.

—
Reply to this email directly or view it on GitHub
#4686 (comment)
.

I should note that I would be perfectly ok with leaving the name as cycler(), especially since its API is identical to the real cycler. It just has added validation.

Keeping the cycler() name seems like the best option I have seen.

I still feel concerned with the name as it feels to generic, where as this function validates specific properties, and thus the name should reflect this specificity...

I see that we either need to make the name more specific, e.g validate_axes_property_cycle, or the function more general e.g. by passing in dict of allowed property names and validators into the function.

what about "pcycler()"? Keep in mind that I want to keep this function
distinct from the "validate_*" group of functions. Yes, it validates stuff,
but so does tons of other things in matplotlib. But this function is not
designed to be used directly as a validator for rcParams.

On Sat, Aug 22, 2015 at 8:37 AM, OceanWolf notifications@github.com wrote:

I still feel concerned with the name as it feels to generic, where as this
function validates specific properties, and thus the name should reflect
this specificity...

I see that we either need to make the name more specific, e.g
validate_axes_property_cycle, or the function more general e.g. by
passing in dict of allowed property names and validators into the function.

—
Reply to this email directly or view it on GitHub
#4686 (comment)
.

I am losing sight of the original reason for calling the function cycler(). The point was that the object's repr would be a valid value for the rcparam. If we were to have it be called cyl() or pcycler() or some_really_long_name(), confusion will ensue due to the repr not looking anything at all like the rcparam. I think I am going to put my foot down and call it cycler(). Now, where to keep this function? Right now, I have it in rcsetup.py, but I don't need to have it there. This thing is tightly coupled with artist properties, so maybe it makes sense to put it in artist.py? Might also be useful to that module to have the beginnings of a aliasing handling system and property validation (not that I am trying to compete with traits, I just like common things to be together).

@WeatherGod Completely agree with you on putting such common things together... I started on the beginnings of a branch that does aliasing, but the branch feels very incomplete, nothing like what I feel the final version should look like... but I don't have the time to work on it right now. I saw my fiddlings as something more for 2.x... after we (me and fariza) have gotten MEP22 and MEP27 out of the way.

I could push it up as a PR now, but as I say it still needs a lot of work to see if it will go the way I picture it going...

I vote to dump it in the top level __init__.py for now (we can always pull it down to another module and import up later if needed) or in cbook.py and just import it from there into where ever we want later.

Hmm, that might work... this cycler function depends upon validators exposed by rcsetup.py. But since __init__.py already imports stuff from rcsetup.py, there doesn't seem to be any issues with circular imports. I'll give that a try.

no dice... __init__.py needs stuff from rcsetup.py, and if I put cycler() in there, then rcsetup.py needs stuff from __init__.py. And I think that is going to be a problem no matter which module I put it in. I guess I'll have to keep it in rcsetup.py... drats.

Latest Travis errors are weird... in py3k:

Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/matplotlib-1.5.dev1-py3.4-linux-x86_64.egg/matplotlib/rcsetup.py", line 713, in validate_cycler
    s = eval(s, {'cycler': cycler})
  File "<string>", line 1, in <module>
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/matplotlib-1.5.dev1-py3.4-linux-x86_64.egg/matplotlib/rcsetup.py", line 688, in cycler
    return reduce(operator.add, (ccycler(k, v) for k, v in validated))
NameError: name 'reduce' is not defined

Did I miss a memo?

from functools import reduce

On Sat, Aug 22, 2015 at 6:17 PM Benjamin Root notifications@github.com
wrote:

Latest Travis errors are weird... in py3k:

Traceback (most recent call last):
File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/matplotlib-1.5.dev1-py3.4-linux-x86_64.egg/matplotlib/rcsetup.py", line 713, in validate_cycler
s = eval(s, {'cycler': cycler})
File "", line 1, in
File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/matplotlib-1.5.dev1-py3.4-linux-x86_64.egg/matplotlib/rcsetup.py", line 688, in cycler
return reduce(operator.add, (ccycler(k, v) for k, v in validated))
NameError: name 'reduce' is not defined

Did I miss a memo?

—
Reply to this email directly or view it on GitHub
#4686 (comment)
.

oy! you can tell I don't do py3k at my day job...

Damn, I liked the idea of putting it in __init__.py. What about making it a private function for now? I.e. _cycler?

@OceanWolf, that is a possibility. With my latest updates, set_prop_cycle() will have a very similar API, and we get validation through that and through the setting of the rcParam (if done as a string). How do others think?

@OceanWolf, I ended up taking your earlier suggestion of having _listify_validator() take an argument to indicate whether or not to allow parsing of a single string into a list, and set that to True only for validate_colorlist(). I needed it for the brand-new hatch pattern validator, which should never be parsed that way.

🍻