-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
DOC: Suppress warnings in visualization.rst #8877
Conversation
|
||
@savefig frame_plot_subplots_layout.png | ||
df.plot(subplots=True, layout=(3, 2), figsize=(6, 6), sharex=False); | ||
df.plot(subplots=True, layout=(2, 3), figsize=(6, 6), sharex=False); |
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.
Is this change so you see better the empty axes?
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 explanation on line 1293 is now no longer correct.
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.
Yes, fixed the text and example.
@sinhrks these warnings also appear when running the tests, so probably there are also some of these calls that can be fixed (see https://travis-ci.org/pydata/pandas/jobs/42314766) |
@sinhrks @jorisvandenbossche this ready to merge? |
just the tests, it would be good to take a look at that as well |
@sinhrks can you see if you can fix the test warnings as well? |
bumbed to 0.16, as you don't visually see it in the docs (warning is not shown by sphinx), so no blocker |
Sure, will fix tests also. |
Updated, and prepared a separate fix #9278 for a program change. |
36ee23b
to
0b75ac9
Compare
@jorisvandenbossche OK, now all the warnings should disapear. Pls check. |
Just looking at the travis log, I still see some warnings. Eg (in the log of the third job: https://travis-ci.org/pydata/pandas/jobs/48128130):
It is not clear to me if you can see which test triggered the warnings. |
|
||
returned = df.plot(subplots=True, ax=axes, layout=(-1, 2)) | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter('ignore') |
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.
Why is the 'catch warnings' needed here? I thought that explicitly stating sharex=False, sharey=False
(as you do in this test) is enough to suppress the warning? (that is what you added in the docs?)
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.
Required, because the test specifies layout
keyword which can't be effective when ax
keyword is passed. I understand these tests was added in #8297, and intend to test layout
is ignored properly.
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.
ah, that makes sense. Maybe explicitely test that the warning is triggered instead of ignoring it?
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.
@sinhrks that sounds about right, but I could have added both the axes
and layout
kws without realizing that the layout would be ignored.
@sinhrks add some questions @TomAugspurger can you also give this a brief look? |
@@ -4594,9 +4594,6 @@ def test_frame_groupby_plot_boxplot(self): | |||
_skip_if_mpl_not_installed() | |||
|
|||
import matplotlib.pyplot as plt | |||
import matplotlib as mpl |
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.
Was this causing warnings as well? I think I've seen warnings about trying to set the backend twice...
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.
Yes it did, thus removed. Do you know the reason why these are added originally?
UserWarning: This call to matplotlib.use() has no effect because the the backend has already been chosen;
matplotlib.use() must be called *before* pylab, matplotlib.pyplot, or matplotlib.backends is
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 reason it causes a warning is because it is called after import matplotlib.pyplot as plt
.
I was looking at the test_graphics, and there they also set the matplotlib backend, but in the test to skip it (https://github.com/sinhrks/pandas/blob/plot_doc/pandas/util/testing.py#L180). So you can also put it in the _skip_if_mpl_not_installed
here. Only you have to import matplotlib in that test, and not matplotlib.pyplot
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.
There is _skip_if_mpl_not_installed
already. And I think matplotlib.pyplot
must be installed here, because continuous test case accesses to pyplot
.
So current fix (remove problematic part) looks OK if there is no specific reason to keep mpl.use
.
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 mean you can put the line in the existing _skip_if_mpl_not_installed
. So adapt _skip_if_mpl_not_installed
to import matplotlib and call mpl.use('Agg')
, and then import pyplot here in the testµµ
I also don't exactly know why it is needed (maybe to run the tests where no graphical toolkit is avaliable?), but since all existing tests for plotting do this, I would keep it that way.
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.
@jorisvandenbossche Understood what you mean. How about moving them to test_graphics.py
to simplify.
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.
Euhm, I would keep it here, as it really tests the plotting method of the GroupBy object. But maybe you could see if you can eg put the _skip_if_mpl_not_installed
in util.testing for reuse.
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.
Currently Groupby.plot
is tested mostly under test_graphics.py
, and plotting tests in test_groupby.py
looks not maintained. Moving remaining to test_graphics
is better for maintenance.
https://github.com/pydata/pandas/blob/master/pandas/tests/test_graphics.py#L2015
Separate issue is test_graphics
gets larger though.
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.
Ah I didn't know you had already a TestDataFrameGroupByPlots
in test_graphics. Yep, certainly OK then to move the tests there.
But is seems a lot of the tests in TestDataFrameGroupByPlots
don't really belong there (as it are just Series.plot tests)
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.
Yeah, better to re-organize test_graphics
. I'll issue separate one.
a6dff05
to
f99a8a7
Compare
@TomAugspurger Thanks to confirm. I think everything done now. |
OK. I've only been able to give it a quick glance, but from that everything looks fine. |
Regarding my comment above (#8877 (comment)) the |
Yes, will do. I originally misunderstood you're referring to different line. |
@sinhrks This is ready to merge for you? In any case, thanks for your endurance, this has been a long one .. |
@jorisvandenbossche Yes. I expect all warnings should be disappears after this and #9464, otherwise will implement a fix in #9464. |
DOC: Suppress warnings in visualization.rst
Thanks! |
Closes #8234. Sorry to take a long.
I understand there are 2 warnings:
This is raised from
mpl
as plotting empty axes for demonstration purpose. Thus simply suppressed.This is a warning displayed when passing multiple axes. Added explanations.