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

DOC: Added sentence to docstring of histogram_bin_edges to explain bin width #18344

Merged
merged 4 commits into from
Feb 10, 2021

Conversation

jamiebarker0310
Copy link
Contributor

This regards #18319 where it was discussed that the width of the bins returned is not equal to what would is described by the formulas described in the "bins" parameter.

@melissawm
Copy link
Member

I think the build failure is unrelated, builds fine locally.

@seberg
Copy link
Member

seberg commented Feb 6, 2021

Yeah, we are seeing that a lot just now:

make: *** [Makefile:179: html-build] Terminated

Too long with no output (exceeded 10m0s): context deadline exceeded

I opened gh-18349, from the docs it sounds like that should fix it (https://support.circleci.com/hc/en-us/articles/360007188574-Build-has-hit-timeout-limit)

@@ -562,7 +562,9 @@ def histogram_bin_edges(a, bins=10, range=None, weights=None):
below, :math:`h` is the binwidth and :math:`n_h` is the number of
bins. All estimators that compute bin counts are recast to bin width
using the `ptp` of the data. The final bin count is obtained from
``np.round(np.ceil(range / h))``.
``np.round(np.ceil(range / h))``. The final bin width is often less
than what is returned by the formulas below, in order to allow the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: "The final bin width will therefore generally be less than what is returned by the estimators." "estimator" is a better term than formula, and more consistent. The part about entire range is covered by ptp two sentences back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment, will adjust and repush

Copy link
Contributor

@madphysicist madphysicist left a comment

Choose a reason for hiding this comment

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

I think this is a good change. Hopefully it gets included soon.

@melissawm melissawm merged commit 7b0653c into numpy:master Feb 10, 2021
@melissawm
Copy link
Member

Thanks, @jamiebarker0310 !

@jamiebarker0310 jamiebarker0310 deleted the histogram-binwidth-doc branch February 10, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants