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

ENH/CLN: add HistPlot class inheriting MPLPlot #7809

Merged
merged 1 commit into from Aug 11, 2014

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Jul 20, 2014

Because hist and boxplot are separated from normal plot, there are some inconsistencies with these functions. Looks better to include them to MPLPlot framework.

Maybe scatter and hist can be deprecated in 0.15 if MPLPlot can offer better GroupBy plot (plan to do in separate PR).

Example

This allows to use kind='hist in DataFrame.plot and Series.plot. (No changes for DataFrame.hist)

import pandas as pd
import numpy as np
import matplotlib.pyplot as plt

df = pd.DataFrame(np.random.randn(1000, 5))
df.plot(kind='hist', subplots=True)

figure_1

df.plot(kind='hist', stacked=True)

figure_2

Remaining Items

  • Add a release note in API section detailing this change/enhancement
  • Modify doc
  • Add tests (both histogram and kde)
  • Check stacked=True can be supported (DataFrame.hist doesn't support it though..).
    • Add tests for stacking

@jreback
Copy link
Contributor

jreback commented Jul 20, 2014

does this change anything? or is just an internal refactor?

what is more consistent?

@jreback
Copy link
Contributor

jreback commented Jul 21, 2014

this independent of #7717 ?

@jreback jreback added this to the 0.15.0 milestone Jul 21, 2014
@shoyer
Copy link
Member

shoyer commented Jul 21, 2014

It looks like the main API change is that as of this PR you can make a histogram with df.plot(kind='hist'), as well as df.hist(). Seems like a win for consistency!

# kinds supported by both dataframe and series
_common_kinds = ['line', 'bar', 'barh', 'kde', 'density', 'area']
_common_kinds = ['line', 'bar', 'barh', 'kde', 'density', 'area', 'hist']
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to add this to the doc strings for plot_series/frame, no?

@jreback
Copy link
Contributor

jreback commented Jul 21, 2014

@sinhrks also let's add a release note in API section detailing this change/enhancement

@sinhrks
Copy link
Member Author

sinhrks commented Jul 24, 2014

Will do. Added example and remaining items on top.

@jreback
Copy link
Contributor

jreback commented Jul 24, 2014

is DataFrame.hist now just a call to df.plot(kind='hist')?

@jreback
Copy link
Contributor

jreback commented Jul 24, 2014

change the title on this as well (the PR), no longer a WIP, rigth?

@sinhrks
Copy link
Member Author

sinhrks commented Jul 25, 2014

Going to leave hist as it is (maybe hist can be deprecate in future). There are some items should be done / considered. Once finished, I'll change the title.

@sinhrks sinhrks changed the title (WIP) ENH/CLN: add HistPlot class inheriting MPLPlot ENH/CLN: add HistPlot class inheriting MPLPlot Jul 27, 2014
@sinhrks
Copy link
Member Author

sinhrks commented Jul 27, 2014

I think this can be reviewed (though rot and fontsize processing relies on #7844)

@jreback
Copy link
Contributor

jreback commented Jul 28, 2014

so #7844 should be merged first? or does this include as well?

@sinhrks
Copy link
Member Author

sinhrks commented Jul 28, 2014

Yes, #7844 should be first.

@jreback
Copy link
Contributor

jreback commented Aug 10, 2014

this looked ok, @sinhrks can you rebase

@TomAugspurger have a look?

@sinhrks
Copy link
Member Author

sinhrks commented Aug 10, 2014

OK, rebased.

@TomAugspurger
Copy link
Contributor

I just gave it a quick look (didn't test, computer was stolen!) and I think it looks good.

Only comment was maybe setting a random seed in the doc example, very minor.

Nice work!

On Aug 10, 2014, at 16:21, Sinhrks notifications@github.com wrote:

OK, rebased.


Reply to this email directly or view it on GitHub.

@jreback
Copy link
Contributor

jreback commented Aug 11, 2014

@sinhrks hmm, needs rebasing again, then ok to merge

@jreback
Copy link
Contributor

jreback commented Aug 11, 2014

ping when pushed

@sinhrks
Copy link
Member Author

sinhrks commented Aug 11, 2014

@jreback Rebased and got green.

jreback added a commit that referenced this pull request Aug 11, 2014
ENH/CLN: add HistPlot class inheriting MPLPlot
@jreback jreback merged commit cd7f09e into pandas-dev:master Aug 11, 2014
@jreback
Copy link
Contributor

jreback commented Aug 11, 2014

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Enhancement Internals Related to non-user accessible pandas implementation Visualization plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants