Skip to content

Conversation

@krsnik93
Copy link
Contributor

Copy link
Contributor

@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


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

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)

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
Contributor

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

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
Contributor

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
Contributor

@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