-
Notifications
You must be signed in to change notification settings - Fork 261
fix tablereport error in histogram when data range is very narrow #2189
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
Changes from all commits
181cb25
1aa2978
9a392d6
2a8ef45
6dc9e54
659a032
8d9d42b
d468e57
814bd23
ee9e552
985b758
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,8 +173,14 @@ def _adjust_fig_size(fig, ax, target_w, target_h): | |
|
|
||
|
|
||
| def _get_range(values, frac=0.2, factor=3.0): | ||
| if np.issubdtype(values.dtype, np.floating): | ||
| finite_values = values[np.isfinite(values)] | ||
| else: | ||
| finite_values = values | ||
| if not len(finite_values): | ||
| return 0, 0 | ||
| min_value, low_p, high_p, max_value = np.quantile( | ||
| values, [0.0, frac, 1.0 - frac, 1.0] | ||
| finite_values, [0.0, frac, 1.0 - frac, 1.0] | ||
| ) | ||
| delta = high_p - low_p | ||
| if not delta: | ||
|
|
@@ -193,7 +199,40 @@ def _get_range(values, frac=0.2, factor=3.0): | |
| return low, high | ||
|
|
||
|
|
||
| def _get_safe_hist_range(values): | ||
| # Get a safe min, max range for the histogram (the 'range' parameter of | ||
| # np.histogram). | ||
| # | ||
| # This handles a corner case where the range of the data is very narrow and | ||
| # there are less than 10 (the default number of bins) representable | ||
| # floating-point numbers between the data min and max (with the precision | ||
| # of the input column dtype). In this case numpy/matplotlib histogram with | ||
| # default parameters fails, so we pass the 'range' parameter to extend it | ||
| # slightly beyond the data range so that it is wide enough to contain 10 | ||
| # bins. | ||
| if not len(values) or not ( | ||
| np.issubdtype(values.dtype, np.floating) | ||
| or np.issubdtype(values.dtype, np.integer) | ||
| ): | ||
| # empty or non-numeric inputs (datetimes) need no special handling. | ||
| return None | ||
| vmin, vmax = values.min(), values.max() | ||
| delta = max(np.spacing(vmin), np.spacing(vmax)) | ||
| if vmax - vmin > 12 * delta: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why 12 here and 6 below?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| # Min and max are far enough to divide the data range into 10 bins, we | ||
| # can keep the default range=None. | ||
| # | ||
| # 12 is a bit conservative, 10 is probably enough, but there is no | ||
| # harm in having the plot range slightly wider than is strictly | ||
| # required. | ||
| return None | ||
| # Extend the range by 12 spacing steps (6 on each side) so the new range | ||
| # can contain the bins. | ||
| return vmin - 6 * delta, vmax + 6 * delta | ||
|
|
||
|
|
||
| def _robust_hist(col, ax=None, color=None): | ||
| result = {} | ||
| col = sbd.drop_nulls(col) | ||
| if sbd.is_float(col): | ||
| # avoid any issues with pandas nullable dtypes | ||
|
|
@@ -211,19 +250,25 @@ def _robust_hist(col, ax=None, color=None): | |
| np_histogram_values = sbd.to_numpy( | ||
| _datetime_encoder.DatetimeEncoder(resolution=None).fit_transform(col) | ||
| ).ravel() | ||
| result["total_seconds_offset"] = np_histogram_values.min() | ||
| np_histogram_values = np_histogram_values - result["total_seconds_offset"] | ||
| else: | ||
| np_histogram_values = values | ||
| low, high = _get_range(values) | ||
| inlier_mask = (low <= values) & (values <= high) | ||
| n_low_outliers = (values < low).sum() | ||
| n_high_outliers = (high < values).sum() | ||
| result = {"n_low_outliers": n_low_outliers, "n_high_outliers": n_high_outliers} | ||
| result.update(n_low_outliers=n_low_outliers, n_high_outliers=n_high_outliers) | ||
| np_histogram_inliers = np_histogram_values[inlier_mask] | ||
| result["bin_counts"], result["bin_edges"] = np.histogram( | ||
| np_histogram_values[inlier_mask] | ||
| np_histogram_inliers, range=_get_safe_hist_range(np_histogram_inliers) | ||
| ) | ||
| if ax is None: | ||
| return result | ||
| n, bins, patches = ax.hist(values[inlier_mask]) | ||
| histogram_inliers = values[inlier_mask] | ||
| n, bins, patches = ax.hist( | ||
| histogram_inliers, range=_get_safe_hist_range(histogram_inliers) | ||
| ) | ||
| n_out = n_low_outliers + n_high_outliers | ||
| if not n_out: | ||
| return result | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,3 +28,24 @@ def test_histogram(): | |
| data = pd.Series([0.0]) | ||
| _, hist = _plotting.histogram(data) | ||
| assert (hist["n_low_outliers"], hist["n_high_outliers"]) == (0, 0) | ||
|
|
||
| # Test a column with values very close to each other. | ||
| # There are less than 10 (default number of histogram bins) representable | ||
| # floating-point numbers in the range, which would cause numpy or | ||
| # matplotlib histogram to raise an exception without appropriate handling | ||
| # in skrub. | ||
| low = np.float32(10.0) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment here explaining what this line is for? |
||
| high = np.nextafter(low, 11.0) | ||
|
|
||
| data = pd.Series([low, high]) | ||
| _, hist = _plotting.histogram(data) | ||
| assert hist["n_low_outliers"] == 0 | ||
| assert hist["n_high_outliers"] == 0 | ||
|
|
||
| # all infinite. +- inf are outliers | ||
| data = pd.Series( | ||
| [None, None, float("nan"), float("-inf"), float("-inf"), float("inf")] | ||
| ) | ||
| _, hist = _plotting.histogram(data) | ||
| assert hist["n_low_outliers"] == 2 | ||
| assert hist["n_high_outliers"] == 1 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep i added comments LMK if it is clearer now