Skip to content

Fix legend color comparisions #10031

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

Merged
merged 3 commits into from
Dec 20, 2017
Merged

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Dec 17, 2017

Alternative to #10030 - Matplotlib colors can be lots of different types, so convert them to rgba arrays before comparing.

skirpichev and others added 2 commits December 17, 2017 14:07
This code was introduced in v2.1.1 and, apparently, wasn't tested, because .get_color() returns array.

Here is an issue example:
sympy/sympy#13730 (comment)
@jklymak
Copy link
Member

jklymak commented Dec 17, 2017

Given that we had bug reports when this as changed, yet the changes passed all the tests, either the tests are incomplete or this is accommodating undocumented use of colours. Can we add a test and or document whatever this is supposed to fix?

@dstansby
Copy link
Member Author

dstansby commented Dec 17, 2017

The problem is the original 'color comparison' code in the figure legend method didn't allow for colors to be numpy arrays. Long term I think this should be avoided by a general function for comparing colors, which I've put in #10032

@jklymak
Copy link
Member

jklymak commented Dec 17, 2017

Yeah, the problem is that this code didn't change in 2.1.1, it just got applied to Figure.legend() as well as axes.legend(). So we broke things by making them more consistent. But I'm still not understanding the use-case.

If in master I do

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()
ax.plot(np.arange(10), color=np.array([0.1, 0.3, 0.4]), label='boo')
fig.legend()
plt.show()

That works fine. So 1-D numpy arrays work OK in master.

The error comes when the user specifies a 2-D array

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()
ax.plot(np.arange(10), color=np.array([[0.1, 0.3, 0.4]]), label='boo')
fig.legend()
plt.show()
ValueError: Invalid RGBA argument: array([[ 0.1,  0.3,  0.4]])

I'd argue specifying a 2-D array is not correct, and if it worked before, that was just a fortunate happenstance. OTOH, plot lets you put a 2-D array in, so I agree legend should as well.

Anyway, upshot is we should add a test to test for 2-D arrays being allowed.

@jklymak
Copy link
Member

jklymak commented Dec 17, 2017

OK, I take it back; the above 2-D array test fails in 2.1.0 and 2.0.2 as well. So I'm not clear what the failure case is here. What color spec failed in 2.1.1 that this fixes?

ping @skirpichev

@skirpichev
Copy link
Contributor

@jklymak, sorry. I can't provide better failing test yet.

@jklymak
Copy link
Member

jklymak commented Dec 17, 2017

OK< let us know.

I can reproduce

ValueError: Invalid RGBA argument: array([[ 0.12156863,  0.46666667,  0.70588235,  1.        ]])

but I get that all the way back to 2.0.2, so I don't think this every worked?

If you could just print c before its passed to legend, that would help clarify things.

@dstansby's fix above seems to work for you, but the minimal test I put in above doesn't work on @dstansby's change. So I am mystified. Most importantly we should make sure we have a test that covers your case and the case that is breaking for sympy/sympy#13730

@skirpichev
Copy link
Contributor

Most importantly we should make sure we have a test that covers your case and the case that is breaking for sympy/sympy#13730

Probably, these testcases should be same, as Diofant and Sympy share some codebase.

@tacaswell tacaswell added this to the v2.1.2 milestone Dec 18, 2017
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Lets hold of merging for a few days to let @skirpichev / the sympy folks to track down a test case.

@jklymak
Copy link
Member

jklymak commented Dec 18, 2017

See here for a link to their efforts: sympy/sympy#13730 (comment)

@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

I'll put in a TST PR for this.

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.

4 participants