[MRG] ENH Permit NaN while allowing to filter out inf in validation tools.#7892
[MRG] ENH Permit NaN while allowing to filter out inf in validation tools.#7892raghavrv wants to merge 2 commits intoscikit-learn:masterfrom
Conversation
amueller
left a comment
There was a problem hiding this comment.
I think this is alright. It adds more complexity to an already complex part of sklearn, though. We might consider not exposing this in check_array and just calling assert_all_finite() in the imputers.
| assert_raises(ValueError, check_array, X_nan) | ||
| check_array(X_inf, force_all_finite=False) # no raise | ||
| # allow_nan check | ||
| check_array(X_nan, force_all_finite=True, allow_nan=True) # no raise |
There was a problem hiding this comment.
this is a bit hard to read, but I guess it's too late to rename force_all_finite.
| copy=False, force_all_finite=True, ensure_2d=True, | ||
| allow_nd=False, ensure_min_samples=1, ensure_min_features=1, | ||
| warn_on_dtype=False, estimator=None): | ||
| copy=False, force_all_finite=True, allow_nan=False, |
There was a problem hiding this comment.
this should go to the end in case someone used non-kwargs, right?
There was a problem hiding this comment.
I don't think we'd usually worry about this... We've previously taken the approach that we assume kwargs is required after unspecified small number of required args...
| Whether to raise an error on np.inf and np.nan in X. | ||
|
|
||
| allow_nan : boolean (default=False) | ||
| Whether to allow nan values in X. |
There was a problem hiding this comment.
But I think you don't currently check in the case that force_all_finite=False and allow_nan=False. Do one of:
- handle this case
- document this behaviour
- make 'allow_nan' a value for
force_all_finiterather than an additional parameter.
| raise ValueError("Input contains NaN, infinity" | ||
| " or a value too large for %r." % X.dtype) | ||
| if allow_nan: | ||
| def any_not_isfinite(X): return np.isinf(X).any() |
There was a problem hiding this comment.
It seems to me there's a not missing here.
3a0a7ab to
ecc5b47
Compare
|
closed by #10459 |
nanbut notinfin validation tools. This will be required as we introduce more methods / imputers permitting missing values as'NaN' / np.nan.For instance currently in imputer this line sets
force_all_finitetoFalseas that is the only way to permit NaN. But this also means that we are lettinginfto pass through without any checks. There is no test currently but this small sample code emphasizes my point. (This is also added as a NRT).In master
In this branch
TODO
allow_nanto selectively permit nan without allowing inf values.allow_nanmissing_valuesis+/-np.inf@tguillemot @agramfort @amueller @jnothman Reviews please? :)
Whether to disallow
np.nanif missing values is not nan is debatable as it seems to break backward compatibility.