Skip to content

[MRG] ENH Permit NaN while allowing to filter out inf in validation tools. #7892

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[MRG] ENH Permit NaN while allowing to filter out inf in validation tools. #7892

wants to merge 2 commits into from

Conversation

raghavrv
Copy link
Member

@raghavrv raghavrv commented Nov 16, 2016 •
edited
Loading

For instance currently in imputer this line sets force_all_finite to False as that is the only way to permit NaN. But this also means that we are letting inf to 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

>>> import numpy as np
>>> from sklearn.preprocessing import Imputer
>>> X = [[np.inf, 8, 9, np.nan], [np.nan, 10, 10, 0], [10, 11, 9, 11]]
# For axis=0, it silently discards column 0, assuming it contains only missing values. An error would be more appropriate in this case.
>>> Imputer(axis=0).fit_transform(X)
array([[  8. ,   9. ,   5.5],
       [ 10. ,  10. ,   0. ],
       [ 11. ,   9. ,  11. ]])

# This error points to the right row but conveys an incorrect message
>>> Imputer(axis=1).fit_transform(X)
ValueError: Some rows only contain missing values: [0]

In this branch

>>> import numpy as np
>>> from sklearn.preprocessing import Imputer
>>> X = [[np.inf, 8, 9, np.nan], [np.nan, 10, 10, 0], [10, 11, 9, 11]]
>>> Imputer(axis=0).fit_transform(X)
ValueError: Input contains infinity or a value too large for dtype('float64').
>>> Imputer(axis=1).fit_transform(X)
ValueError: Input contains infinity or a value too large for dtype('float64').

TODO

  • Add allow_nan to selectively permit nan without allowing inf values.
  • Fix imputer to use allow_nan
  • Add NRT to check raised errors.
  • Handle the case when missing_values is +/-np.inf

@tguillemot @agramfort @amueller @jnothman Reviews please? :)

Whether to disallow np.nan if missing values is not nan is debatable as it seems to break backward compatibility.

@raghavrv raghavrv mentioned this pull request Nov 16, 2016
Closed
4 tasks
amueller
amueller reviewed Nov 16, 2016
View reviewed changes
Copy link
Member

@amueller amueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sklearn/utils/tests/test_validation.py
@@ -141,6 +141,11 @@ def test_check_array():
X_nan[0, 0] = np.nan
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit hard to read, but I guess it's too late to rename force_all_finite.

sklearn/utils/validation.py
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go to the end in case someone used non-kwargs, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes indeed! Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

jnothman
jnothman requested changes Nov 16, 2016
View reviewed changes
sklearn/utils/validation.py
@@ -83,16 +90,19 @@ def as_float_array(X, copy=True, force_all_finite=True):
force_all_finite : boolean (default=True)
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_finite rather than an additional parameter.

sklearn/utils/validation.py
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me there's a not missing here.

@raghavrv raghavrv mentioned this pull request Dec 18, 2016
Closed
@raghavrv raghavrv force-pushed the permit_nan_in_check_array branch from 3a0a7ab to ecc5b47 Compare December 18, 2016 13:48
@glemaitre glemaitre mentioned this pull request Jan 11, 2018
Closed
@TomDLT
Copy link
Member

TomDLT commented Jan 18, 2018

closed by #10459

@TomDLT TomDLT closed this Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman requested changes

@amueller amueller amueller left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@raghavrv @TomDLT @jnothman @amueller