Repository: scikit-learn/scikit-learn
Pull request: [MRG+2] Fix sparse simple imputer
Fixes #11495 

Changed the fit in `SimpleImputer` to properly handle sparse input with explicit zeros.
I made it a little more clean also (I guess)

Since the SimpleImputer hasn't been released yet, do I still have do add an entry in the what's new for this bug fix ?

Problem statement:
BUG: SimpleImputer gives wrong result on sparse matrix with explicit zeros
The current implementation of the `SimpleImputer` can't deal with zeros stored explicitly in sparse matrix.
Even when stored explicitly, we'd expect that all zeros are treating equally, right ?
See for example the code below:
```python
import numpy as np
from scipy import sparse
from sklearn.impute import SimpleImputer

X = np.array([[0,0,0],[0,0,0],[1,1,1]])
X = sparse.csc_matrix(X)
X[0] = 0    # explicit zeros in first row

imp = SimpleImputer(missing_values=0, strategy='mean')
imp.fit_transform(X)

>>> array([[0.5, 0.5, 0.5],
           [0.5, 0.5, 0.5],
           [1. , 1. , 1. ]])
```
Whereas the expected result would be
```python
>>> array([[1. , 1. , 1. ],
           [1. , 1. , 1. ],
           [1. , 1. , 1. ]])
```

Hints from issue discussion:

Commit message: add regression test

Patch to review:
diff --git a/sklearn/impute.py b/sklearn/impute.py
index 836bfb0167add..bc76c7f53af7e 100644
--- a/sklearn/impute.py
+++ b/sklearn/impute.py
@@ -241,80 +241,51 @@ def fit(self, X, y=None):
 
     def _sparse_fit(self, X, strategy, missing_values, fill_value):
         """Fit the transformer on sparse data."""
-        # Count the zeros
-        if missing_values == 0:
-            n_zeros_axis = np.zeros(X.shape[1], dtype=int)
-        else:
-            n_zeros_axis = X.shape[0] - np.diff(X.indptr)
+        mask_data = _get_mask(X.data, missing_values)
+        n_implicit_zeros = X.shape[0] - np.diff(X.indptr)
 
-        # Mean
-        if strategy == "mean":
-            if missing_values != 0:
-                n_non_missing = n_zeros_axis
-
-                # Mask the missing elements
-                mask_missing_values = _get_mask(X.data, missing_values)
-                mask_valids = np.logical_not(mask_missing_values)
-
-                # Sum only the valid elements
-                new_data = X.data.copy()
-                new_data[mask_missing_values] = 0
-                X = sparse.csc_matrix((new_data, X.indices, X.indptr),
-                                      copy=False)
-                sums = X.sum(axis=0)
-
-                # Count the elements != 0
-                mask_non_zeros = sparse.csc_matrix(
-                    (mask_valids.astype(np.float64),
-                     X.indices,
-                     X.indptr), copy=False)
-                s = mask_non_zeros.sum(axis=0)
-                n_non_missing = np.add(n_non_missing, s)
-
-            else:
-                sums = X.sum(axis=0)
-                n_non_missing = np.diff(X.indptr)
+        statistics = np.empty(X.shape[1])
 
-            # Ignore the error, columns with a np.nan statistics_
-            # are not an error at this point. These columns will
-            # be removed in transform
-            with np.errstate(all="ignore"):
-                return np.ravel(sums) / np.ravel(n_non_missing)
+        if strategy == "constant":
+            return np.full(X.shape[1], fill_value)
 
-        # Median + Most frequent + Constant
         else:
-            # Remove the missing values, for each column
-            columns_all = np.hsplit(X.data, X.indptr[1:-1])
-            mask_missing_values = _get_mask(X.data, missing_values)
-            mask_valids = np.hsplit(np.logical_not(mask_missing_values),
-                                    X.indptr[1:-1])
-
-            # astype necessary for bug in numpy.hsplit before v1.9
-            columns = [col[mask.astype(bool, copy=False)]
-                       for col, mask in zip(columns_all, mask_valids)]
-
-            # Median
-            if strategy == "median":
-                median = np.empty(len(columns))
-                for i, column in enumerate(columns):
-                    median[i] = _get_median(column, n_zeros_axis[i])
-
-                return median
-
-            # Most frequent
-            elif strategy == "most_frequent":
-                most_frequent = np.empty(len(columns))
-
-                for i, column in enumerate(columns):
-                    most_frequent[i] = _most_frequent(column,
-                                                      0,
-                                                      n_zeros_axis[i])
-
-                return most_frequent
-
-            # Constant
-            elif strategy == "constant":
-                return np.full(X.shape[1], fill_value)
+            for i in range(X.shape[1]):
+                column = X.data[X.indptr[i]:X.indptr[i+1]]
+                mask_column = mask_data[X.indptr[i]:X.indptr[i+1]]
+                column = column[~mask_column]
+
+                # combine explicit and implicit zeros if missing_values != 0
+                if missing_values != 0:
+                    mask_zeros = _get_mask(column, 0)
+                    column = column[~mask_zeros]
+                    n_explicit_zeros = mask_zeros.sum()
+                    n_zeros = n_implicit_zeros[i] + n_explicit_zeros
+                else:
+                    n_zeros = n_implicit_zeros[i]
+
+                if strategy == "mean":
+                    if missing_values == 0:
+                        s = column.size
+                    else:
+                        s = column.size + n_zeros
+                    statistics[i] = np.nan if s == 0 else column.sum() / s
+
+                elif strategy == "median":
+                    if missing_values == 0:
+                        statistics[i] = _get_median(column, 0)
+                    else:
+                        statistics[i] = _get_median(column,
+                                                    n_zeros)
+
+                elif strategy == "most_frequent":
+                    if missing_values == 0:
+                        statistics[i] = _most_frequent(column, 0, 0)
+                    else:
+                        statistics[i] = _most_frequent(column,
+                                                       0,
+                                                       n_zeros)
+        return statistics
 
     def _dense_fit(self, X, strategy, missing_values, fill_value):
         """Fit the transformer on dense data."""
diff --git a/sklearn/tests/test_impute.py b/sklearn/tests/test_impute.py
index f5c42f7443487..cf7bd4adf3b74 100644
--- a/sklearn/tests/test_impute.py
+++ b/sklearn/tests/test_impute.py
@@ -705,3 +705,22 @@ def test_chained_imputer_additive_matrix():
                              random_state=rng).fit(X_train)
     X_test_est = imputer.transform(X_test)
     assert_allclose(X_test_filled, X_test_est, atol=0.01)
+
+
+def regression_test_sparse_zeros():
+    # regression test on wrong statistics computation for sparse input with
+    # explicit zeros. (#11495)
+    X = np.array([[0, 0, 0],
+                  [0, 0, 0],
+                  [1, 1, 1]])
+    X = sparse.csc_matrix(X)
+    X[0] = 0
+
+    X_true = np.array([[1, 1, 1],
+                       [1, 1, 1],
+                       [1, 1, 1]])
+
+    imputer = SimpleImputer(missing_values=0, strategy='mean')
+    X_trans = imputer.fit_transform(X)
+
+    assert_array_equal(X_trans, X_true)


Code review comments:
diff_hunk: @@ -705,3 +705,22 @@ def test_chained_imputer_additive_matrix():
                              random_state=rng).fit(X_train)
     X_test_est = imputer.transform(X_test)
     assert_allclose(X_test_filled, X_test_est, atol=0.01)
+
+
+def regression_test_sparse_zeros():
+    # regression test on wrong statistics computation for sparse input with
+    # explicit zeros. (#11495)
+    X = np.array([[0, 0, 0],
+                  [0, 0, 0],
+                  [1, 1, 1]])
+    X = sparse.csc_matrix(X)
+    X[0] = 0
+
path: sklearn/tests/test_impute.py
text: @user1:
to show that this is checking the right thing, you can assert that X.nnz > 3

diff_hunk: @@ -705,3 +705,22 @@ def test_chained_imputer_additive_matrix():
                              random_state=rng).fit(X_train)
     X_test_est = imputer.transform(X_test)
     assert_allclose(X_test_filled, X_test_est, atol=0.01)
+
+
+def regression_test_sparse_zeros():
path: sklearn/tests/test_impute.py
text: @user1:
tests must start with `test_`