-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: fix matplotlib warning on CN color #36981
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
Refer to commit aaa69d1, where the way of treating CN colors was originally developed. This is not relevant anymore as more readable alternative is provided.
CI checks failure due to pyx files, not because of my change. |
# check whether the string can be convertible to single color | ||
maybe_single_color = _maybe_valid_colors([colors]) | ||
# check whether each character can be convertible to colors | ||
maybe_color_cycle = _maybe_valid_colors(list(colors)) |
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.
are these not relevant / texted? (e.g. the axes.prop_cycle) or is this handled by conv.to_rbga now?
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.
I guess that this was the way of handling compatibility with matplotlib <= 2.0.
If the color 'CN' is passed, then it would pick N-th color from "axes.prop_cycle".
Before matplotlib 2.0 there were no CN colors, so this is probably why this construction is here.
pandas/plotting/_matplotlib/style.py
Outdated
|
||
def _is_cn_color(color: str) -> bool: | ||
"""Check if color string is CN color, like 'C0', 'C1', etc.""" | ||
cn_colors = ["C" + str(x) for x in range(10)] |
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.
The default matplotlib color cycle has 10 colors, but if set differently this test won't be valid.
pandas/plotting/_matplotlib/style.py
Outdated
False otherwise. | ||
""" | ||
conv = matplotlib.colors.ColorConverter() | ||
if _is_cn_color(color): |
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.
to_rgba
handles the "Cn"
style, so why is this separate check needed?
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.
Correct! It turns out that we can make it even cleaner.
conv.to_rgba handles CN colors itself.
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.
interesting! so now matplotlib handles all color validations? one minor coment
pandas/plotting/_matplotlib/style.py
Outdated
hex_color = [c["color"] for c in list(plt.rcParams["axes.prop_cycle"])] | ||
colors = [hex_color[int(colors[1])]] | ||
elif maybe_single_color: | ||
if _is_single_color(colors): |
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.
maybe put it in one line?
`if isinstance(colors, str) and _is_single_color(colors)`
also nice to put an issue number below this line for future reference!
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.
Done
Just confirming that the concerned warning is not raised anymore.
The other warnings are handled here #36982 |
For some reason the test fails in Travis CI.
|
I would only think that the error happened because of some other deprecation warning for that particular environment. |
Looks like now everything works. The new test fails on master branch, but succeeds with this PR changes. |
emm, just make sure it's not a flaky error, maybe try to rebase one more time to see if the failure comes back? btw, does this need a whatsnew? probably no, right? |
All checks pass. Neither the warning was raised (I checked all Travis logs) nor the test fail (like it should if this warning is raised). |
lgtm. @charlesdong1991 merge if good here. |
this is fine |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff