Skip to content

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

Merged
merged 12 commits into from
Oct 10, 2020

Conversation

ivanovmg
Copy link
Member

@ivanovmg ivanovmg commented Oct 8, 2020

@ivanovmg ivanovmg requested a review from jreback October 8, 2020 13:48
@ivanovmg
Copy link
Member Author

ivanovmg commented Oct 8, 2020

CI checks failure due to pyx files, not because of my change.

@jreback jreback added the Visualization plotting label Oct 8, 2020
@jreback jreback added this to the 1.2 milestone Oct 8, 2020
@jreback jreback added the Clean label Oct 8, 2020
# 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))
Copy link
Contributor

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?

Copy link
Member Author

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.

@jreback
Copy link
Contributor

jreback commented Oct 8, 2020

cc @charlesdong1991


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)]
Copy link
Contributor

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.

False otherwise.
"""
conv = matplotlib.colors.ColorConverter()
if _is_cn_color(color):
Copy link
Contributor

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?

Copy link
Member Author

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.
Copy link
Member

@charlesdong1991 charlesdong1991 left a 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

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):
Copy link
Member

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@ivanovmg
Copy link
Member Author

ivanovmg commented Oct 9, 2020

Just confirming that the concerned warning is not raised anymore.

(base) root@f31dd4516194:/workspaces/pandas# pytest pandas/tests/plotting/test_frame.py
======================================================================================================================== test session starts =========================================================================================================================
platform linux -- Python 3.7.7, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
rootdir: /workspaces/pandas, configfile: setup.cfg
plugins: cov-2.10.0, forked-1.2.0, xdist-1.34.0, hypothesis-5.23.11, asyncio-0.12.0
collected 186 items                                                                                                                                                                                                                                                  

pandas/tests/plotting/test_frame.py ...........................x.................................................................................................x............................................................                                 [100%]

========================================================================================================================== warnings summary ==========================================================================================================================
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_plot
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_plot
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_plot
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_timeseries
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_timeseries
pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_errorbar_timeseries
  /workspaces/pandas/pandas/plotting/_matplotlib/__init__.py:61: UserWarning: To output multiple subplots, the figure containing the passed axes is being cleared
    plot_obj.generate()

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================================================================================================= 184 passed, 2 xfailed, 6 warnings in 62.41s (0:01:02) ========================================================================================================
(base) root@f31dd4516194:/workspaces/pandas#

The other warnings are handled here #36982

@ivanovmg
Copy link
Member Author

ivanovmg commented Oct 9, 2020

For some reason the test fails in Travis CI.

=================================== FAILURES ===================================

_________________ TestDataFramePlots.test_mpl2_color_cycle_str _________________

[gw0] linux -- Python 3.7.8 /home/travis/miniconda3/envs/pandas-dev/bin/python

self = <pandas.tests.plotting.test_frame.TestDataFramePlots object at 0x7f022d75ca90>
    def test_mpl2_color_cycle_str(self):
        # GH 15516
        df = DataFrame(randn(10, 3), columns=["a", "b", "c"])
        colors = ["C0", "C1", "C2", "C3", "C4", "C5", "C6", "C7", "C8", "C9"]
        with warnings.catch_warnings(record=True) as w:
            # GH 36972
            warnings.simplefilter("always", "MatplotlibDeprecationWarning")

            for color in colors:
                _check_plot_works(df.plot, color=color)

            no_warning_raised = len(w) == 0
>           assert no_warning_raised, "MatplotlibDeprecationWarning was raised"
E           AssertionError: MatplotlibDeprecationWarning was raised
E           assert False

pandas/tests/plotting/test_frame.py:183: AssertionError

@ivanovmg
Copy link
Member Author

ivanovmg commented Oct 9, 2020

I would only think that the error happened because of some other deprecation warning for that particular environment.
I thus changed assertion to match the actual deprecation warning message. We'll see how it goes.

@ivanovmg
Copy link
Member Author

ivanovmg commented Oct 9, 2020

Looks like now everything works. The new test fails on master branch, but succeeds with this PR changes.

@charlesdong1991
Copy link
Member

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?

@ivanovmg
Copy link
Member Author

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).
Regarding the whatsnew - I don't know. What would others think?

@jreback
Copy link
Contributor

jreback commented Oct 10, 2020

lgtm. @charlesdong1991 merge if good here.

@jreback
Copy link
Contributor

jreback commented Oct 10, 2020

this is fine

@jreback jreback merged commit 0435d91 into pandas-dev:master Oct 10, 2020
@ivanovmg ivanovmg deleted the warnings/matplotlib branch October 20, 2020 11:37
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: warning when using colors 'CN'
4 participants