-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix using .get_color() and friends in labels handling #10030
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)
I don't think this works because (for example) string colors are valid, and |
It does work for me (e.g. fix plotting tests in the Diofant), but apparently it breaks some matplotlib tests.
Unfortunately, no. Now I see this exception:
(This test coming from https://github.com/diofant/diofant/blob/master/diofant/plotting/tests/test_plot.py). |
@skirpichev Can you provide a minimal test that exercises the failing path? |
@tacaswell, I'll try to do this. |
I've updated my PR so now it should hopefully work with arrays, give it another try @skirpichev |
It would be nice to see the use case that is breaking. |
@jklymak, see this comment. |
I saw that comment. Sorry this broke something for you. What is the code that triggers the error? |
@dstansby. yes - updated solution seems to be working. |
@skirpichev From how you are using |
I think the sympy folks are looking into it now. |
@jklymak I thought my PR superseeded this one? |
I was confused too. I think somehow the prs were linked and yours was committed on top of this one versus being separate. |
Your commits follow this one; ergo merging it also means this is merged. There is no linkage except the commits. |
FWIW #10064 will wipe them both out ;-) Reviews welcome.... |
PR Summary
This code was introduced in v2.1.1 and, apparently, wasn't tested, because .get_color() returns array.
Here is examples (raises ValueError):
sympy/sympy#13730 (comment)
or
https://travis-ci.org/diofant/diofant/jobs/316670834
PR Checklist