-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Fix .hist and .plot.hist when passing existing figure (#37278) #37467
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
Conversation
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.
Thanks @krsnik93 for the PR!
Not super familiar with this part of the code base but looks good - some comments re: whatsnew & tests
doc/source/whatsnew/v1.2.0.rst
Outdated
@@ -476,6 +476,7 @@ Plotting | |||
|
|||
- Bug in :meth:`DataFrame.plot` was rotating xticklabels when ``subplots=True``, even if the x-axis wasn't an irregular time series (:issue:`29460`) | |||
- Bug in :meth:`DataFrame.plot` where a marker letter in the ``style`` keyword sometimes causes a ``ValueError`` (:issue:`21003`) | |||
- Bug in :meth:`DataFrame.hist`, :meth:`DataFrame.plot.hist` and :meth:`Series.plot.hist` when passing an existing figure (:issue:`37278`) |
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.
this should go in enhancements I think. Something like
:meth:`DataFrame.hist`, :meth:`DataFrame.plot.hist` can now be called with an existing matplotlib figure object via added ``figure`` argument
Possibly add an example (not 100% that's needed though)
@@ -152,6 +152,13 @@ def test_hist_with_legend_raises(self, by): | |||
with pytest.raises(ValueError, match="Cannot use both legend and label"): | |||
s.hist(legend=True, by=by, label="c") | |||
|
|||
def test_hist_with_passed_figure(self): |
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'd call this test_hist_with_figure_argument
@@ -395,6 +402,13 @@ def test_hist_with_legend_raises(self, by, column): | |||
with pytest.raises(ValueError, match="Cannot use both legend and label"): | |||
df.hist(legend=True, by=by, column=column, label="d") | |||
|
|||
def test_hist_with_passed_figure(self): |
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'd call this test_hist_with_figure_argument
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
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.
@krsnik93 sorry we've been quiet on this - would you mind merging master & resolving conflicts (it's been a while)
looks good to me but I don't know this part of the code base well
@charlesdong1991 can you take a look |
closing as stale, if you want to continue working, please ping. |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff