Skip to content

TST: DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout() #15515

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 27, 2017
Merged

TST: DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout() #15515

merged 4 commits into from
Feb 27, 2017

Conversation

ResidentMario
Copy link
Contributor

@jreback jreback changed the title Add unit test for #9351 TST: DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout() Feb 27, 2017
@jreback jreback added Testing pandas testing functions or related to the test suite Visualization plotting labels Feb 27, 2017
@ResidentMario
Copy link
Contributor Author

Note: in addition to the #9351 unit test this PR also adds a _skip_if_mpl_1 method to the test utilities.

jreback
jreback reviewed Feb 27, 2017
View reviewed changes
pandas/util/testing.py Outdated
v = matplotlib.__version__
if v < LooseVersion('2.0.0') or v[0] == '0' or v[0] == '1':
import pytest
pytest.skip("matplotlib 1.5")
Copy link
Contributor

Choose a reason for hiding this comment

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

matpotlib < 2.0.0

jreback
jreback reviewed Feb 27, 2017
View reviewed changes
pandas/tests/plotting/test_hist_method.py Outdated
df.plot.hist()

try:
self.plt.tight_layout()
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the try/except (if it works it won't raise)

add a 1-line comment & the issue number as a comment

jreback
jreback requested changes Feb 27, 2017
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

couple of small comments

jreback
jreback reviewed Feb 27, 2017
View reviewed changes
pandas/util/testing.py Outdated
@@ -286,6 +286,14 @@ def _skip_if_mpl_1_5():
pytest.skip("matplotlib 1.5")


def _skip_if_mpl_1():
import matplotlib
Copy link
Contributor

@jreback jreback Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

actually you don't need this, instead use:

if self.mpl_ge_2_0_0:

as these are defined in the base class (you can look in pandas.tools.plotting

Copy link
Contributor Author

@ResidentMario ResidentMario Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

self.mpl_get_2_0_0 I assume? Edit: nvm, I see it is indeed ge. What does that stand for?

Copy link
Contributor

Choose a reason for hiding this comment

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

no take this function out

@ResidentMario
Copy link
Contributor Author

I rewrote the test using mpl_ge_2_0_0 and removed the aux method. pytest isn't a name in test_hist_method, and I don't see a generic skip in testing.py, so I've just made the test conditional, e.g. there's no explicit skip. Let me know if that's OK or not.

@codecov-io
Copy link

codecov-io commented Feb 27, 2017 •
edited
Loading

Codecov Report

Merging #15515 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15515      +/-   ##
==========================================
+ Coverage   90.36%   90.36%   +<.01%     
==========================================
  Files         136      136              
  Lines       49553    49554       +1     
==========================================
+ Hits        44780    44781       +1     
  Misses       4773     4773
Impacted Files Coverage Δ
pandas/core/generic.py 96.31% <0%> (ø) ✅
pandas/types/cast.py 85.45% <0%> (+0.02%) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3ae4c7...809d847. Read the comment docs.

jreback
jreback reviewed Feb 27, 2017
View reviewed changes
pandas/tests/plotting/test_hist_method.py Outdated
if self.mpl_ge_2_0_0:
df = DataFrame(randn(100, 2))
df.plot.hist()
self.plt.tight_layout()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a _check_plot_works here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would _check_plot_works(df.hist); self.plt.tight_layout() work? The method seems to be designed for testing parameter inputs, but tight_layout isn't a histogram parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's fine. I realize this just raises AttributeError, so that would be caught by the tests anyhow.

@jreback
Copy link
Contributor

jreback commented Feb 27, 2017

lgtm. ping on green.

@jreback jreback added this to the 0.20.0 milestone Feb 27, 2017
@jreback
Copy link
Contributor

jreback commented Feb 27, 2017

actually, pls add a whatsnew note as well (I guess in Bug Fixes). This is really a mpl compat issue, and there isn't much we can do about it with < 2.0.0, so best just to note this in the whatsnew as well so readers can react to this.

@jreback
Copy link
Contributor

jreback commented Feb 27, 2017

lgtm. thanks.

@jreback jreback merged commit fed1827 into pandas-dev:master Feb 27, 2017
@jreback
Copy link
Contributor

jreback commented Feb 27, 2017

thanks!

@ResidentMario ResidentMario deleted the 9351 branch February 27, 2017 20:43
jorisvandenbossche added a commit that referenced this pull request Mar 13, 2017
@jorisvandenbossche
DOC: correct whatsnew note of #15515
03dca96
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017
@ResidentMario @AnkurDedania
TST: DataFrame.hist() does not get along with matplotlib.pyplot.tight…
a38a957 
…_layout() (pandas-dev#15515)

* Add unit test for pandas-dev#9351

* Tweaks.

* add _check_plot_works; rm aux method

* Add whatsnew entry.
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017
@jorisvandenbossche @AnkurDedania
DOC: correct whatsnew note of pandas-dev#15515
3bca592
mattip pushed a commit to mattip/pandas that referenced this pull request Apr 3, 2017
@jorisvandenbossche @mattip
DOC: correct whatsnew note of pandas-dev#15515
a8509c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Testing pandas testing functions or related to the test suite Visualization plotting
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.

DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout()
3 participants
@ResidentMario @codecov-io @jreback