Skip to content
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

Improve color cycle usage #5674

Closed
wants to merge 11 commits into from
Closed

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Dec 14, 2015

Addresses #5489 and #5577 and the minutes from the meeting on 2015-11-30.

@mdboom mdboom added this to the next major release (2.0) milestone Dec 14, 2015
clist = rcParams['axes.color_cycle']
prop_cycler = cycler('color', clist)

cycler = cls._get_property_cycler()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clobbering cycler.

Copy link
Member Author

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.

@WeatherGod
Copy link
Member

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
Copy link
Member

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).

Copy link
Member Author

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.

@mdboom mdboom mentioned this pull request Dec 14, 2015
3 tasks
clist = rcParams['axes.color_cycle']
prop_cycler = cycler('color', clist)

colors = prop_cycler.by_key()['color']
Copy link
Member Author

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).

@mdboom mdboom force-pushed the color-cycle-handling branch 2 times, most recently from 84a80db to ced56dd Compare December 16, 2015 14:18
@mdboom
Copy link
Member Author

mdboom commented Dec 16, 2015

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.

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.

plt.plot(x, y, 'b')
plt.scatter(x0, y0, s0, color='b')

We could also solve this by having some sort of grouping context:

with same_color():
   plt.plot(x, y)
   plt.scatter(x0, y0, s0)

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.

This also does not preclude my counter-proposal of attaching labels to iterations of the prop_cycler for reuse in subsequent plots.

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.

else:
fc = kwargs.pop('fc', kwargs.pop('facecolor', None))
lw = kwargs.pop('lw', kwargs.pop('linewidth', None))
if fc is None:
Copy link
Member

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.

Copy link
Member Author

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)
Copy link
Member

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]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mdboom
Copy link
Member Author

mdboom commented Dec 30, 2015

I guess we need to figure out what to do with this one:

  1. Include for 2.0
  2. Postpone for post-2.0 when a better solution emerges

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.

@WeatherGod
Copy link
Member

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 rcParams['axes.color_cycle']. It is still possible to do that (albeit with a deprecation warning now).

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?

@mdboom
Copy link
Member Author

mdboom commented Dec 30, 2015

Before, if one were doing that, they would have had to use rcParams['axes.color_cycle']. It is still possible to do that (albeit with a deprecation warning now).

I was thinking more that users used to use 'b', 'r' and 'g' etc., as many of our own examples do.

@mdboom
Copy link
Member Author

mdboom commented Jan 25, 2016

Note to self: This should also handle violinplot.

@mdboom
Copy link
Member Author

mdboom commented Apr 11, 2016

Replaced by #6291.

@mdboom mdboom closed this Apr 11, 2016
@QuLogic QuLogic modified the milestones: unassigned, 2.0 (style change major release) Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants