-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: Check Warnings in test_graphics_others #13188
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
CLN: Check Warnings in test_graphics_others #13188
Conversation
@neirbowj I don't know if this will fix your failing test or not, could you give it a shot? I think we have some assertions in here that depends on the test execution order :/ |
@TomAugspurger Yes, the tests are now passing. Thank you. |
@TomAugspurger can you rebase this and see if you can fix the error? |
Been trying to figure it out, but I can't get the error locally :/ I'll ping you when I get it sorted out. |
@TomAugspurger ping :-) |
f6d36f5
to
7e68600
Compare
Current coverage is 84.33%@@ master #13188 diff @@
==========================================
Files 138 138
Lines 51111 51111
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 43107 43107
Misses 8004 8004
Partials 0 0
|
with tm.assert_produces_warning(UserWarning): | ||
_check_plot_works(self.ts.hist, by=self.ts.index.month) | ||
with tm.assert_produces_warning(UserWarning): | ||
_check_plot_works(self.ts.hist, by=self.ts.index.month, bins=5) |
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.
These ones are not generating a warning (and I also don't see why they should?)
7e68600
to
db82165
Compare
Maybe you can put the second commit for mpl compat in a separate PR if that is less of a problem. |
077b329
to
4882dd6
Compare
@jorisvandenbossche OK I'm giving up on this one I think :) Locally I get
Not really sure what about my environment that's different than Travis, but it's green now so I'm good with this if you are. |
you need to run more tests - prob some other test is suppressing the warnings keep running more and more till they fail |
4882dd6
to
5e656d2
Compare
|
5e656d2
to
8f1a459
Compare
@TomAugspurger The "UserWarning: To output multiple subplots, the figure containing the passed axes is being cleared" you give is a very strange warning for that test, as there are no axes passed at all. Translated, the test does this:
I do not get any warning for that locally if I run this in a console |
I also get those warnings if I run that single test with nosetests. But I see that |
Ah, I didn't realize that I can drop in some comments explaining what's going on. |
@TomAugspurger I didn't know as well. Good idea to add some comments why the catching of a warning is needed, then this should be good to merge |
8f1a459
to
b466fc1
Compare
@jorisvandenbossche this is ready to go in I think. |
Merged! @TomAugspurger Did it actually close #13185 ? (at the top, there is a 'maybe') |
git diff upstream/master | flake8 --diff
Also a MatplotlibDeprecationWarning for use of
.get_axes()
vs..axes
in some tests.