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

BUG: Fix .hist and .plot.hist when passing existing figure (#37278) #37467

Closed
wants to merge 2 commits into from

Conversation

krsnik93
Copy link
Contributor

Copy link
Member

@arw2019 arw2019 left a 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

@@ -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`)
Copy link
Member

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):
Copy link
Member

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):
Copy link
Member

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

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Nov 29, 2020
Copy link
Member

@arw2019 arw2019 left a 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

@jbrockmendel
Copy link
Member

@charlesdong1991 can you take a look

@jreback
Copy link
Contributor

jreback commented Oct 4, 2021

closing as stale, if you want to continue working, please ping.

@jreback jreback closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Undocumented, Inconsistent matplotlib figure usage (Series.hist vs Series.plot.hist)
4 participants