Repository: scikit-learn/scikit-learn
Pull request: [MRG] Make imputers picklable
#### Reference Issues/PRs
Fixes #11462 


#### What does this implement/fix? Explain your changes.

Changes the problematic line (identified in the issue) to properly identify `np.nan`s after pickling. 

Test added to `util.estimator_checks` to check that all estimators that allow `np.nan`s properly pickle models that are fitted with data containing `np.nan`s


Problem statement:
np.nan is not np.nan after serialization causing SimpleImputer to fail transforming properly
#### Description
When I load fitted ChaninedImputer instance from `joblib` or `pickle`, it drops columns with nans instead of imputing them when I call `.fit_transform` or `.transform`.  

#### To reproduce:
```python
In [1]: import joblib
In [2]: import numpy as np
In [3]: from sklearn.impute import ChainedImputer
In [4]: d = np.arange(25).reshape(5, 5).astype(np.float32)
In [5]: np.fill_diagonal(d, np.nan)
In [6]: d
Out[6]: 
array([[nan,  1.,  2.,  3.,  4.],
       [ 5., nan,  7.,  8.,  9.],
       [10., 11., nan, 13., 14.],
       [15., 16., 17., nan, 19.],
       [20., 21., 22., 23., nan]], dtype=float32)
In [7]: imputer = ChainedImputer()
In [8]: imputer.fit_transform(d) # correct behavior
Out[8]: 
array([[ 0.06354573,  1.        ,  2.        ,  3.        ,  4.        ],
       [ 5.        ,  6.0265727 ,  7.        ,  8.        ,  9.        ],
       [10.        , 11.        , 12.038444  , 13.        , 14.        ],
       [15.        , 16.        , 17.        , 18.081451  , 19.        ],
       [20.        , 21.        , 22.        , 23.        , 23.929924  ]],
      dtype=float32)

In [9]: joblib.dump(imputer, "imputer.joblib")
In [10]: imputer = joblib.load("imputer.joblib")
In [11]: imputer.fit_transform(d) # wrong behavior, all columns are dropped as all columns have nans
Out[11]: array([], shape=(5, 0), dtype=float32)
In [12]: e = np.arange(10).reshape(2, 5).astype(np.float32)
In [13]: np.fill_diagonal(e, np.nan)
In [14]: e
Out[14]: 
array([[nan,  1.,  2.,  3.,  4.],
       [ 5., nan,  7.,  8.,  9.]], dtype=float32)
In [15]: imputer.fit_transform(e) # wrong behavior, columns with nans are dropped
Out[15]: 
array([[2., 3., 4.],
       [7., 8., 9.]], dtype=float32)
```
#### Expected results
In these examples, the inputer should replace nans with the appropriate number

#### Actual results
The columns with nans are being dropped 

#### Versions
Linux-4.15.0-24-generic-x86_64-with-debian-stretch-sid
Python 3.6.4 |Anaconda, Inc.| (default, Jan 16 2018, 18:10:19) 
[GCC 7.2.0]
NumPy 1.14.5
SciPy 1.1.0
Scikit-Learn 0.20.dev0



<!-- Thanks for contributing! -->

Hints from issue discussion:
interesting.... ping @sergeyf

I can reproduce and the issue is related to `SimpleImputer` actually.
It comes from the following [line](https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/impute.py#L45)

`np.nan` should be cached but the pickling/unpickling should create a new instance which make the `is np.nan` condition to fail. The solution would be to replace the line by 

```python
if isinstance(value_to_mask, float) and np.isnan(value_to_mask):
```

We need a regression test as well. It should consist on fitting an imputer, dumping and loading it and fitting again and check that `statistics_` is equal.
Actually, I would have think that the following test should have to that:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L1155


Oh yes, the test actually do not have NaN specifically. So we need a specific regression test for this case.
Thanks for the ping. @glemaitre I am not following this 100%. Is this bug my fault? Should I try to fix it?
yes it should. that check, and others, should have some NaNs thrown in when
the estimator can handle them.

good diagnosis. I think we should follow the "never check NaN with is"
rule...

you're welcome to work on it @sergeyf but the ping was a false alarm

Ah, thanks. I think I'll pass on this one as I have a swamp of work-for-money to get through =)
:) I don't pity you!

Commit message: add test to check that estimators that allow nans properly pickle and unpickle when fitted with input data with nans

Patch to review:
diff --git a/sklearn/impute.py b/sklearn/impute.py
index 836bfb0167add..2f093dd24fae9 100644
--- a/sklearn/impute.py
+++ b/sklearn/impute.py
@@ -42,7 +42,7 @@
 
 def _get_mask(X, value_to_mask):
     """Compute the boolean mask X == missing_values."""
-    if value_to_mask is np.nan:
+    if isinstance(value_to_mask, float) and np.isnan(value_to_mask):
         if X.dtype.kind == "f":
             return np.isnan(X)
         elif X.dtype.kind in ("i", "u"):
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index 02d91ee80791b..aa9a7e4ef3ad7 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -1190,6 +1190,30 @@ def check_estimators_pickle(name, estimator_orig):
         unpickled_result = getattr(unpickled_estimator, method)(X)
         assert_allclose_dense_sparse(result[method], unpickled_result)
 
+    # check if input with nans pickles properly
+    if name in ALLOW_NAN:
+        # set random 10 elements to be corrupted
+        X.ravel()[np.random.choice(X.size, 10, replace=False)] = np.nan
+        estimator.fit(X, y)
+
+        nan_result = dict()
+        for method in check_methods:
+            if hasattr(estimator, method):
+                nan_result[method] = getattr(estimator, method)(X)
+
+        # pickle and unpickle!
+        pickled_estimator = pickle.dumps(estimator)
+        if estimator.__module__.startswith('sklearn.'):
+            assert_true(b"version" in pickled_estimator)
+        unpickled_estimator = pickle.loads(pickled_estimator)
+
+        for method in nan_result:
+            unpickled_nan_result = getattr(unpickled_estimator, method)(X)
+            if name == 'ChainedImputer':
+                assert_allclose_dense_sparse(nan_result[method], unpickled_nan_result, rtol=1e-5, atol=0.1)
+            else:
+                assert_allclose_dense_sparse(nan_result[method], unpickled_nan_result)
+
 
 @ignore_warnings(category=(DeprecationWarning, FutureWarning))
 def check_estimators_partial_fit_n_features(name, estimator_orig):


Code review comments:
diff_hunk: @@ -1190,6 +1190,30 @@ def check_estimators_pickle(name, estimator_orig):
         unpickled_result = getattr(unpickled_estimator, method)(X)
         assert_allclose_dense_sparse(result[method], unpickled_result)
 
+    # check if input with nans pickles properly
+    if name in ALLOW_NAN:
+        # set random 10 elements to be corrupted
+        X.ravel()[np.random.choice(X.size, 10, replace=False)] = np.nan
+        estimator.fit(X, y)
+
+        nan_result = dict()
+        for method in check_methods:
+            if hasattr(estimator, method):
+                nan_result[method] = getattr(estimator, method)(X)
+
+        # pickle and unpickle!
+        pickled_estimator = pickle.dumps(estimator)
+        if estimator.__module__.startswith('sklearn.'):
+            assert_true(b"version" in pickled_estimator)
+        unpickled_estimator = pickle.loads(pickled_estimator)
+
+        for method in nan_result:
+            unpickled_nan_result = getattr(unpickled_estimator, method)(X)
+            if name == 'ChainedImputer':
+                assert_allclose_dense_sparse(nan_result[method], unpickled_nan_result, rtol=1e-5, atol=0.1)
path: sklearn/utils/estimator_checks.py
text: @user1:
Shouldn't this pas without the increased tolerance if this aims to fix https://github.com/scikit-learn/scikit-learn/issues/11462 ? or am I missing something? 

@author:
I believe the tolerance is okay, there's a similar tolerance for this [test](https://github.com/scikit-learn/scikit-learn/blob/3e19d7f6d78f3e28f2dd1fef08ad7d410e5ecd72/sklearn/tests/test_impute.py#L655).

diff_hunk: @@ -1190,6 +1190,30 @@ def check_estimators_pickle(name, estimator_orig):
         unpickled_result = getattr(unpickled_estimator, method)(X)
         assert_allclose_dense_sparse(result[method], unpickled_result)
 
+    # check if input with nans pickles properly
+    if name in ALLOW_NAN:
+        # set random 10 elements to be corrupted
path: sklearn/utils/estimator_checks.py
text: @user1:
```
set random 10 elements to be NaN
```
"corrupted" seems a bit strong here

diff_hunk: @@ -1190,6 +1190,30 @@ def check_estimators_pickle(name, estimator_orig):
         unpickled_result = getattr(unpickled_estimator, method)(X)
         assert_allclose_dense_sparse(result[method], unpickled_result)
 
+    # check if input with nans pickles properly
+    if name in ALLOW_NAN:
+        # set random 10 elements to be corrupted
+        X.ravel()[np.random.choice(X.size, 10, replace=False)] = np.nan
+        estimator.fit(X, y)
+
+        nan_result = dict()
+        for method in check_methods:
+            if hasattr(estimator, method):
+                nan_result[method] = getattr(estimator, method)(X)
+
+        # pickle and unpickle!
+        pickled_estimator = pickle.dumps(estimator)
+        if estimator.__module__.startswith('sklearn.'):
+            assert_true(b"version" in pickled_estimator)
path: sklearn/utils/estimator_checks.py
text: @user1:
That sounds unrelated to detecting nans. Why is it necessary?

@author:
yep, I removed the unnecessary portion

diff_hunk: @@ -1190,6 +1190,30 @@ def check_estimators_pickle(name, estimator_orig):
         unpickled_result = getattr(unpickled_estimator, method)(X)
         assert_allclose_dense_sparse(result[method], unpickled_result)
 
+    # check if input with nans pickles properly
+    if name in ALLOW_NAN:
+        # set random 10 elements to be corrupted
+        X.ravel()[np.random.choice(X.size, 10, replace=False)] = np.nan
path: sklearn/utils/estimator_checks.py
text: @user1:
Use an RNG for reproducibility,
```py
rng = np.random.RandomState(42)
mask = rng.choice(X.size ..
```

also `X.reshape(-1)` might be preferable. From the `numpy.ravel` documentation,
> When a view is desired in as many cases as possible, arr.reshape(-1) may be preferable