-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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)
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? |
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 |
Yeah, the problem is that this code didn't change in 2.1.1, it just got applied to 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()
I'd argue specifying a 2-D array is not correct, and if it worked before, that was just a fortunate happenstance. OTOH, Anyway, upshot is we should add a test to test for 2-D arrays being allowed. |
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 |
@jklymak, sorry. I can't provide better failing test yet. |
OK< let us know. I can reproduce
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 |
Probably, these testcases should be same, as Diofant and Sympy share some codebase. |
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.
Lets hold of merging for a few days to let @skirpichev / the sympy folks to track down a test case.
See here for a link to their efforts: sympy/sympy#13730 (comment) |
I'll put in a TST PR for this. |
Backport PR #10031 on branch v2.1.x
Alternative to #10030 - Matplotlib colors can be lots of different types, so convert them to rgba arrays before comparing.