Skip to content

Implicit addition of "color" to property_cycle breaks semantics #6380

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.

Sign up for GitHub

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

Jump to bottom
Closed
WeatherGod opened this issue May 7, 2016 · 9 comments
Closed

Implicit addition of "color" to property_cycle breaks semantics #6380

WeatherGod opened this issue May 7, 2016 · 9 comments
Assignees
@tacaswell
Labels
API: default changes
Milestone
v2.0.0

Comments

@WeatherGod
Copy link
Member

In the 2.x branch, the recent addition of the CN feature also added an implicit assumption that "color" will always be in the property cycle. This conflicts with the feature of prop_cycle that it only advances for a plotting function if the user does not specify at least one of the properties made available by the property cycle.

This is important because several plotting functions will make multiple calls to plot()/fill() under the hood, but should have already advanced the cycle. Also, this mimics pre-1.5 behavior of the color_cycle where it did not advance if color was already specified.

The test (which is currently being modified to paper over the problem) which noticed this change is the following:

    fig, ax = plt.subplots()
    ax.set_prop_cycle('linewidth', [2, 4])
    for c in range(1, 4):
        ax.plot(np.arange(10), c * np.arange(10), lw=0.1)
    ax.plot(np.arange(10), 4 * np.arange(10))
    ax.plot(np.arange(10), 5 * np.arange(10))

In the v1.5.x branch, this would result in the cycler not being advanced for the first three lines, and then two lines are made with linewidths of 2 and 4 in that order. In 2.x branch, the cycler gets advanced because of the implicit inclusion of "color" to the property cycle. Therefore, the cycler has a property which is not specified by the user, and it thinks it needs to advance itself. This results in a plot that has three thin lines and then a line of width 4 and then 2 (because of the odd number of plot calls prior).

Now, consider the following scenario:

    fig, ax = plt.subplots()
    ax.set_prop_cycle('color', 'rb')
    for c in range(1, 4):
        ax.plot(np.arange(10), c * np.arange(10), color='k')
    ax.plot(np.arange(10), 4 * np.arange(10))
    ax.plot(np.arange(10), 5 * np.arange(10))

In this case, the cycler will not advance for the first three plots, but will for the last two -- as expected. This difference in behavior will cause confusion among users, and will be very difficult to explain.

I shall re-iterate my design concept behind the property-cycle feature that I added for v1.5: "Color is not special!". I am fine with having a failover mechanism for "C0" in the case that color isn't in the cycler, but to actually modify the cycler provided by the user only leads to confusion. What would we do when we want to expand the "CN" notation to other properties? Are we going to start implicitly include them into the property cycle as well?
@WeatherGod WeatherGod mentioned this issue May 7, 2016
Merged
@WeatherGod WeatherGod added this to the 2.0 (style change major release) milestone May 7, 2016
@tacaswell
Copy link
Member

Trying to address this brought up an other corner case.

In the test we do

ax.set_prop_cycle(linewidth=[2, 3, 4, 5, 6], facecolor='bgcmy')

so if we remove the implicit color plot falls back to C0 which in the color module looks at the prop_cycle in rcparams.

I think this is just a wart of how this, as the C0 explicitly is a global thing. If you want to change the meaning you have to use a context manager.

The other option is to start injecting cyclers into every call to to_rgba which seems less than good.

@efiring
Copy link
Member

efiring commented May 23, 2016

On 2016/05/22 5:37 PM, Thomas A Caswell wrote:

so if we remove the implicit color |plot| falls back to |C0| which in
the |color| module looks at the prop_cycle in |rcparams|.

I'm not sure I see how this is a problem--do you mean it is because the
user is expecting to see plot use the full cycle from rcparams, but
instead it keeps repeating the C0 from rcparams? Or the user is
expecting facecolor to override the missing color?

I think this is just a wart of how this, as the |C0| explicitly is a
global thing. If you want to change the meaning you have to use a
context manager.

The other option is to start injecting cyclers into every call to
|to_rgba| which seems less than good.

Downright unacceptable.

@tacaswell
Copy link
Member

If the user sets an axes level prop cycle, the CN notation will still refer
to the global prop cycle which could be confusing.

We could also move some if the color conversion machinery to the axes
objects to give them a chance to deal with the CN before it gets resolved
globally.

On Mon, May 23, 2016, 00:04 Eric Firing notifications@github.com wrote:

On 2016/05/22 5:37 PM, Thomas A Caswell wrote:

so if we remove the implicit color |plot| falls back to |C0| which in
the |color| module looks at the prop_cycle in |rcparams|.

I'm not sure I see how this is a problem--do you mean it is because the
user is expecting to see plot use the full cycle from rcparams, but
instead it keeps repeating the C0 from rcparams? Or the user is
expecting facecolor to override the missing color?

I think this is just a wart of how this, as the |C0| explicitly is a
global thing. If you want to change the meaning you have to use a
context manager.

The other option is to start injecting cyclers into every call to
|to_rgba| which seems less than good.

Downright unacceptable.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6380 (comment)

@efiring
Copy link
Member

efiring commented May 28, 2016

Would it be possible to add a cycler kwarg to to_rgba and to_rgba_array? If None, the rcParams cycler would be used. Every call to one of these functions from a function with an Axes would substitute the cycler from that Axes.

@tacaswell
Copy link
Member

That seems reasonable to me.

attn @anntzer

@anntzer
Copy link
Contributor

anntzer commented May 29, 2016

Actually I have no idea of what's happening in the "interesting" block of to_rgba:

    if _is_nth_color(c):
        from matplotlib import rcParams
        from matplotlib.rcsetup import cycler
        prop_cycler = rcParams['axes.prop_cycle']
        if prop_cycler is None and 'axes.color_cycle' in rcParams:
            clist = rcParams['axes.color_cycle']
            prop_cycler = cycler('color', clist)
        colors = prop_cycler._transpose().get('color', 'k')
        c = colors[int(c[1]) % len(colors)]

which I just copy-pasted from the previous implementation... so I'll let someone else take care of it.

@tacaswell tacaswell mentioned this issue May 29, 2016
Closed
@efiring
Copy link
Member

efiring commented May 29, 2016

@tacaswell, that block of code looks a little odd: if there is neither a 'axes.prop_cycle' nor a 'axes.color_cycle', prop_cycler will be None, and the call to ._transpose() will fail. Does this need to be trapped with a more informative exception? If rcsetup guarantees that this cannot occur, then a comment to that effect would be helpful. Actually, if that is the case, then the test for 'axes.color_cycle' in rcParams is not needed.

@tacaswell
Copy link
Member

The rc validation means rcParams['axes.prop_cycle'] can never be None. Would we want to fall back to the color cycle if color is not in prop_cycle? I think not because that means the user changed the default prop_cycle and we should respect that.

@tacaswell tacaswell mentioned this issue May 29, 2016
Merged
@tacaswell tacaswell self-assigned this May 29, 2016
@tacaswell
Copy link
Member

closed by #6499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tacaswell tacaswell

Labels
API: default changes
Projects
None yet
Milestone
v2.0.0
Development

No branches or pull requests
4 participants
@efiring @tacaswell @WeatherGod @anntzer