-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Improve color cycle usage #5674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
clist = rcParams['axes.color_cycle'] | ||
prop_cycler = cycler('color', clist) | ||
|
||
cycler = cls._get_property_cycler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clobbering cycler
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry -- this is some weird merge thing. Will fix.
Just for the record, I am still very hesitant on this idea, particularly because is it specific to colors, to the exclusion of any other property type. If this was generalized to be able to access one or more members from the cycler, I might warm up to the idea, but I still feel that this is wrong somehow. Perhaps it would make more sense to allow some sort of interpolation feature into rcParams (a la configobject)? That way, users could define rcParam entries that are just simply list of values and they would also define the prop_cycle string using those entries. Then, they have complete access to the original list of values for use while also allowing prop_cycle to be dynamically defined in the rcParams. This also does not preclude my counter-proposal of attaching labels to iterations of the prop_cycler for reuse in subsequent plots. The currently proposed approach would make such a feature more difficult to implement in the future, I think. |
For example: | ||
|
||
- `[0]`: The first color in the cycle | ||
- `[1]`: The second color in the cycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably note that negative indexes are not allowed (as evidenced by the regex used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I should also mention the fact that it wraps around.
aaedcb8
to
c9b9b8a
Compare
clist = rcParams['axes.color_cycle'] | ||
prop_cycler = cycler('color', clist) | ||
|
||
colors = prop_cycler.by_key()['color'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @tacaswell mentioned, this can be done in a backward compatible way (that doesn't require the latest version of cycler as this does).
84a80db
to
ced56dd
Compare
I'm not opposed to that idea, but the point here is to have a super convenient syntax for accessing a color cycle. The use case is very common here, where people currently have a few things that are "grouped" that they want to plot all in the same color. Currently people tend to use the single character color names for this, but in doing so, they lose any advantage of having a nice set of colors that are specifically designed to have high contrast from each other, look good in print and screen etc.
We could also solve this by having some sort of grouping context:
but that is asking users to change their habits a lot and would be significantly more invasive to implement. As nice as the cycler concept is, it does break down when trying to combine filled and unfilled things.
I still like that suggestion in theory, but in practice it creates ambiguity about whether the user wants just the color or the whole set of styles from the current cycler "dictionary". I'd argue there's still a strong use for the former which this PR is trying to address. |
ced56dd
to
91906bc
Compare
else: | ||
fc = kwargs.pop('fc', kwargs.pop('facecolor', None)) | ||
lw = kwargs.pop('lw', kwargs.pop('linewidth', None)) | ||
if fc is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a path through this where fc is never defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Fixed.
colors = ('b', 'g', 'r', 'c', 'm', 'y', 'k', 'w') | ||
get_next_color = self._get_patches_for_fill.get_next_color | ||
else: | ||
color_cycler = itertools.cycle(colors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[bikeshed]color_cycle
instead of color_cycler
as it is a cycle object not a cycler object (sorry, I am really bad at naming things)[/bikeshed]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I guess we need to figure out what to do with this one:
Either way we need to do something to address the problem we've created by making it harder to address the colors in the default color cycle. |
Well, my vote is that this should be for 2.1 because it is a new feature. I also reject the notion that accessing the color cycle is any harder than it was before. Before, if one were doing that, they would have had to use What I am against is the assumption that color is the only thing that users will care about and the only thing that deserves a shortcut. This notation limits limits the possibilities for other properties, making any shorthand notations for them more difficult. Perhaps it would make more sense to simply get rid of the deprecation warning? |
I was thinking more that users used to use |
Note to self: This should also handle violinplot. |
Replaced by #6291. |
Addresses #5489 and #5577 and the minutes from the meeting on 2015-11-30.