Skip to content

WIP: Use color cycle in more places #5584

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

Closed
wants to merge 7 commits into from

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Nov 30, 2015

This fixes some plot types that were using their own hardcoded color cycle to use the color cycle in the rcParams.

I'm submitting this PR basically to demonstrate the motivation for #5577 which would fix this in a more uniform and less hackish way. Though I'm fine with merging this as-is if we decide that doing #5577 properly is too disruptive right now.

@mdboom mdboom added this to the next major release (2.0) milestone Nov 30, 2015
@@ -2554,9 +2555,20 @@ def pie(self, x, explode=None, labels=None, colors=None,
raise ValueError("'label' must be of length 'x'")
if len(x) != len(explode):
raise ValueError("'explode' must be of length 'x'")
if colors is None:
if ((colors is None) and
('color' in self._get_patches_for_fill._prop_keys)):
Copy link
Member

Choose a reason for hiding this comment

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

indent to not line up with codeblock

@WeatherGod
Copy link
Member

There is already a test file for testing property cycles, so it would make sense to add the new test there.

Also, I am lamenting a bit that this is only utilizing the color portion of the property cycle. I really would like to see more properties used, and the internal.classic_mode trick might be a good way to shim that into more places.

Also, could we check that switching from a classic style to a different style doesn't leave that _internal.classic_mode param behind? That would probably cause bugs down the road, I think.

@mdboom
Copy link
Member Author

mdboom commented Nov 30, 2015

Also, I am lamenting a bit that this is only utilizing the color portion of the property cycle.

Unfortunately, the property cycle that makes sense for line plots will not be appropriate for other kinds of plots. I don't think there's any reasonable way to resolve that. We could have separate property cycles for different plot types (not a great solution either).

Also, could we check that switching from a classic style to a different style doesn't leave that _internal.classic_mode param behind?

If a new style is applied over top, it will remain, but if a new style is set it isn't. I think that's probably the correct behavior.

@WeatherGod
Copy link
Member

Unfortunately, the property cycle that makes sense for line plots will not be appropriate for other
kinds of plots. I don't think there's any reasonable way to resolve that. We could have separate
property cycles for different plot types (not a great solution either).

Right, that is why the current codebase is a bit of a mess in this regard, and it is also partly why the code for handling the munging of properties from the cycler and properties explicitly provided from the user is a bit icky for plot() and bar().

However, the actual function for munging that information is all there in _makeline() and _makefill(). It might make sense to factor out those methods and a few other property-cycling methods from the var-arg handling code so that it can be reused in places where there is no special argument handling needed.

@mdboom
Copy link
Member Author

mdboom commented Nov 30, 2015

However, the actual function for munging that information is all there in _makeline() and _makefill(). It might make sense to factor out those methods and a few other property-cycling methods from the var-arg handling code so that it can be reused in places where there is no special argument handling needed.

Yes, I agree, and I'm sorry I wasn't clear about that -- that's what I'm suggesting we do in #5577. This is really just a proof-of-concept that not doing something like that is a mess.

@tacaswell
Copy link
Member

Given our discussion about how to expose the current Cycler back out to the user, do we want to add it is a new property on to the Axes object (as it is currently held in both of the two _process_plot_var_args objects) or just refer back to one of them?

@mdboom
Copy link
Member Author

mdboom commented Dec 1, 2015

Does anyone know why the two _process_plot_var_args instances exist (rather than just one)? I think that means they each have their own iterator (over the same cycler object) so will advance separately. Is that intentional, or just an artifact? I think it better to have a single iterator that always advances, but maybe I'm missing the reason they both exist. As to where it lives, I don't think it much matters whether it's directly or indirectly referenced by the Axes, as long as there's only one and importantly only one active iterator.

@mdboom
Copy link
Member Author

mdboom commented Dec 1, 2015

To prevent further confusion, I'm going to close this issue, since I never meant it as a serious solution -- just an example of where things need to be fixed in a better way. We can keep the discussion going here, though.

@WeatherGod
Copy link
Member

The two instances was so that if bar() was called and then plot(), they
would each advance their own instance of the color cycle, keeping them "in
sync". Because plot() and bar() form the basis of most of the other
plotting functions, this actually worked really well, especially for more
complex plots that combine patches and lines.

On Tue, Dec 1, 2015 at 8:05 AM, Michael Droettboom <notifications@github.com

wrote:

Closed #5584 #5584.


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

@tacaswell
Copy link
Member

I think (but am not sure why) there are two so that you can do plt.plot(); plt.fill() and they will walk in sync?

I am not seeing where bar uses the second color cycle, it looks like it is only used in Axes.fill.

@WeatherGod
Copy link
Member

You are probably right, I am working off of memory here. I don't use bar()
or fill() all that much. The point is that it allows for two plotting
functions that may typically be called together in a loop (or with multiple
inputs) to be "in sync" regardless of how they are called.

On Tue, Dec 1, 2015 at 10:09 AM, Thomas A Caswell notifications@github.com
wrote:

I think (but am not sure why) there are two so that you can do plt.plot();
plt.fill() and they will walk in sync?

I am not seeing where bar uses the second color cycle, it looks like it
is only used in Axes.fill.


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

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.

3 participants