Skip to content
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

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

Merged
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.

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

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

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

@@ -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

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

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

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.

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!

jorisvandenbossche added a commit that referenced this pull request Mar 13, 2017
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017
…_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
mattip pushed a commit to mattip/pandas that referenced this pull request Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite Visualization plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout()
3 participants