Skip to content

Conversation

@bluesquall
Copy link

Add a histogram method to pandas.Series -- just a thin wrapper around np.histogram.

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@pep8speaks
Copy link

Hello @bluesquall! Thanks for submitting the PR.

@bluesquall
Copy link
Author

Apparently there were 4 more pull requests while I was pushing this, so the note in whatsnew shows an incorrect PR number. I'll update this once the rest of the tests are complete.

@gfyoung gfyoung added this to the No action milestone Nov 11, 2018
@gfyoung gfyoung added the Visualization plotting label Nov 11, 2018
@gfyoung
Copy link
Member

gfyoung commented Nov 11, 2018

@bluesquall : Thank you for opening this PR! We already have a histogram method (Series.hist), so unfortunately, this PR is actually unnecessary.

In the future, I would recommend that you open an issue before opening the PR, as that's where we work out whether there's any need to address whatever is at hand.

@gfyoung gfyoung closed this Nov 11, 2018
@jreback
Copy link
Contributor

jreback commented Nov 11, 2018

.hist is for plotting the histogram
but this indeed is better done with pd.cut

@bluesquall
Copy link
Author

@gfyoung: This returns the counts and bins edges. It doesn't generate the plot, so it isn't directly about visualization (like @jreback noted).

I'm using it for preprocessing with a few more steps before a plot, usually. Sometimes it's just for processing.

@jreback: I did play with pandas.cut a bit before submitting this PR. If I recall correctly, it required a few extra steps to get the output to the form I needed, while simply wrapping numpy.histogram didn't.

I'm happy to open an issue later this week to discuss further and to give some examples.

If you would rather have it implemented using pandas.cut, I'm open to that, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants