TST dtype dependent rtol, atol in assert_allclose_dense_sparse #13978
Conversation
Check for closeness of new_result and the previously compute result should depend on the dtype of the result, and can not be less than that precision's epsilon.
|
is this build hung? |
|
@ogrisel @jeremiedbb CI is green. Could you review, please. |
|
Shouldn't we adjust the |
|
Yes, if that case how about setting |
|
Yes that's fine |
e654d07 to
1073501
Compare
1073501 to
35ca88b
Compare
|
@jeremiedbb Done. |
sklearn/utils/estimator_checks.py
Outdated
| if np.issubdtype(new_result.dtype, np.floating): | ||
| tol = np.finfo(new_result.dtype).eps | ||
| else: | ||
| tol = np.finfo(np.float64).eps |
There was a problem hiding this comment.
maybe add a comment # integer dtype because it's not clear at first sight.
I wonder if it would not be even clearer if you reformulate this way:
if new_result.dtype == np.float32:
tol = np.finfo(np.float32).eps
else:
tol = np.finfo(np.float64).epswdyt ?
There was a problem hiding this comment.
I think using np.floating is much cleaner NumPy, it accounts for np.half, np.longdouble etc., even though these types may not be used in Scikit-Learn.
|
pinging reviewers, please. |
sklearn/utils/estimator_checks.py
Outdated
| @@ -2448,4 +2448,12 @@ def check_fit_idempotent(name, estimator_orig): | |||
| for method in check_methods: | |||
| if hasattr(estimator, method): | |||
| new_result = getattr(estimator, method)(X_test) | |||
| assert_allclose_dense_sparse(result[method], new_result) | |||
| if np.issubdtype(new_result.dtype, np.floating): | |||
| tol = np.finfo(new_result.dtype).eps | |||
There was a problem hiding this comment.
No actually for float64, this would 2.22e-16 do we actually need such precision? I think the previous default of 1e-7 was enough? For other dtypes, I agree.
Maybe then,
# previous defaults in `assert_allclose_dense_sparse`
rtol = max(tol, 1e-7)
atol = max(tol, 1e-9)also maybe take 2*np.finfo(dtype).eps just to be safe?
There was a problem hiding this comment.
Thanks for the good feedback. I agree, and have pushed both proposed changes.
Parameter tol is set to be 2*np.finfo(dtype).eps, rather than np.finfo(dtype).eps. Setting of rtol and atol parameters of `assert_allclose_dense_sparse` is modified to not go below the default values.
|
Thanks! |
Check for closeness of new_result and the previously computed result
in
check_fit_idempotentshould depend on the dtype of the result, andcannot be less than that precision's epsilon.
Reference Issues/PRs
Closes #13977
What does this implement/fix? Explain your changes.
This change sets
atolkeyword in the call toassert_allclose_dense_sparseincheck_fit_idempotentestimator checker.Any other comments?
@amueller @jeremiedbb