fix tablereport error in histogram when data range is very narrow#2189
Conversation
jeromedockes
added
bug
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Thanks @jeromedockes
The code looks mostly good to me, but I'd like a few more comments explaining what is happening before we merge this
| return None | ||
| vmin, vmax = values.min(), values.max() | ||
| delta = max(np.spacing(vmin), np.spacing(vmax)) | ||
| if vmax - vmin > 12 * delta: |
There was a problem hiding this comment.
if the range is already wider than 12, we don't need to adjust it. otherwise we add 6 on each side so it is at least 12-wide afterwards.
probably 5 + 5 = 10 would be enough because there are 10 bins, but we don't care and have a little safety margin to make sure we don't crash
| return low, high | ||
|
|
||
|
|
||
| def _get_safe_hist_range(values): |
There was a problem hiding this comment.
could you explain this function in a bit more detail? it's not clear what is happening in the code and what is being returned
There was a problem hiding this comment.
yep i added comments LMK if it is clearer now
| _, hist = _plotting.histogram(data) | ||
| assert (hist["n_low_outliers"], hist["n_high_outliers"]) == (0, 0) | ||
|
|
||
| low = np.float32(10.0) |
There was a problem hiding this comment.
can you add a comment here explaining what this line is for?
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Thanks for the comments @jeromedockes, the PR looks good to me
2 participants
we can get an error in matplotlib / numpy histogram if the column is not constant but the data range does not contain enough representable floating numbers for the default number of bins
for example it would fail on a column like this