MAINT: Tidy np.histogram, and improve error messages #9889
Conversation
d72f995 to
96dace9
Compare
| @@ -646,7 +647,7 @@ def histogram(a, bins=10, range=None, normed=False, weights=None, | |||
| a = asarray(a) | |||
| if weights is not None: | |||
| weights = asarray(weights) | |||
| if np.any(weights.shape != a.shape): | |||
| if weights.shape != a.shape: | |||
There was a problem hiding this comment.
No need for np.any here - these are tuples
numpy/lib/function_base.py
Outdated
| @@ -671,34 +672,65 @@ def histogram(a, bins=10, range=None, normed=False, weights=None, | |||
| mn -= 0.5 | |||
| mx += 0.5 | |||
|
|
|||
| # density overrides the normed keyword | |||
| if density is not None: | |||
| normed = False | |||
There was a problem hiding this comment.
A little tidier to handle this first
|
|
||
| # parse the overloaded bins argument | ||
| n_equal_bins = None | ||
| bin_edges = None |
There was a problem hiding this comment.
Using these names makes things a lot clearer, rather than bins meaning different things in different places
numpy/lib/function_base.py
Outdated
| elif np.ndim(bins) == 1: | ||
| bin_edges = np.asarray(bins) | ||
| if np.any(bin_edges[:-1] > bin_edges[1:]): | ||
| raise ValueError('`bins` must increase monotonically') |
There was a problem hiding this comment.
Moved from below - previously this was needlessly checked for a n_bins input if the weights were weird.
There was a problem hiding this comment.
Looks like 'non-decreasing', not monotonically increasing.
There was a problem hiding this comment.
Message wasn't touched by this CR, but I guess I may as well fix it while I'm here. Can you suggest the replacement message?
There was a problem hiding this comment.
NVM, I checked, and monotonic is the correct word.
96dace9 to
0f42555
Compare
numpy/lib/function_base.py
Outdated
| try: | ||
| n_equal_bins = operator.index(bins) | ||
| except TypeError: | ||
| raise TypeError('If a scalar, `bins` must be an integer') |
There was a problem hiding this comment.
Maybe 'If `bins` is a scalar, it must be an integer'
Same for next error message.
There was a problem hiding this comment.
Or perhaps `bins` must be an integer, if [it is ]a scalar, to try and start all the messages with bins?
There was a problem hiding this comment.
Let me think about it a bit more.
There was a problem hiding this comment.
Thinking again, this particular error message should be the generic one - see my update,
numpy/lib/function_base.py
Outdated
| raise ValueError('`bins` must increase monotonically') | ||
|
|
||
| else: | ||
| raise ValueError("If an array, `bins` must be 1d") |
There was a problem hiding this comment.
See above about error messages.
numpy/lib/function_base.py
Outdated
| else: | ||
| raise ValueError("If an array, `bins` must be 1d") | ||
|
|
||
| del bins |
There was a problem hiding this comment.
No, but it enforces that the code below does not use the overloaded variable any more, which is probably a good thing
numpy/lib/function_base.py
Outdated
|
|
||
| # compute the bins if only the count was specified | ||
| if n_equal_bins is not None: | ||
| bin_edges = linspace(mn, mx, n_equal_bins + 1, endpoint=True) |
There was a problem hiding this comment.
Maybe binmin, binmax instead of mn, mx?
There was a problem hiding this comment.
Not all they're used for though - nevermind, seems reasonable - perhaps (upper|lower)_edge, (first|last)_edge,or bin_edge_(upper|lower)?
There was a problem hiding this comment.
(first|last) _edge sounds good.
Split up the overloaded `bins` variable into separate names depending on its meaning Helpful errors are now emitted for: * non-integer bin counts (fixes numpygh-8072) * non-1d bin edges Removes another use of `np.isscalar`...
0f42555 to
5b7b87e
Compare
|
Thanks Eric. |
Split up the overloaded
binsvariable into separate names depending on its meaningHelpful errors are now emitted for:
Removes another use of
np.isscalar...Had to read through this to work out the purpose of the
np.scalar, so seemed worth fixing up at the same time