Repository: scikit-learn/scikit-learn
Pull request: BUG fix SparseCoder to follow scikit-learn API and allow cloning
#### Reference Issues/PRs
Fixes #8675
Fixes #16336
Supersedes and closes #16346

#### What does this implement/fix? Explain your changes.
SparseCoder cannot be used in a GridSearchCV because it could not be cloned. A parameter in the constructor had a different meaning when stored in an instance (dictionary).

Problem statement:
Problem with SparseCoder Object, dictionary not assigned
## Code

```
import numpy as np
from sklearn.decomposition import SparseCoder
sparsecoding = SparseCoder(dictionary=np.random.random((4,8)), n_jobs=2)
print(sparsecoding)
```

## Output
```
SparseCoder(dictionary=None, n_jobs=2, split_sign=False,
      transform_algorithm='omp', transform_alpha=None,
      transform_n_nonzero_coefs=None)
```

'dictionary' parameter is not assigned, only 'components_'. This causes GridSearchCV function to drop an error when called using a pipeline, with this object inside. :
```
/opt/anaconda/lib/python3.6/site-packages/sklearn/decomposition/dict_learning.py in __init__(self, dictionary, transform_algorithm, transform_n_nonzero_coefs, transform_alpha, split_sign, n_jobs)
    899                  transform_n_nonzero_coefs=None, transform_alpha=None,
    900                  split_sign=False, n_jobs=1):
--> 901         self._set_sparse_coding_params(dictionary.shape[0],
    902                                        transform_algorithm,
    903                                        transform_n_nonzero_coefs,

AttributeError: 'NoneType' object has no attribute 'shape'
```

SparseCoder does not follow the scikit-learn API and will break in 0.24
The `SparseCoder` does not follow the scikit-learn API.

```python
d = np.array([[1, 2, 3], [1, 2, 3]])
sc = SparseCoder(dictionary=d)
sc.get_params()
/home/glemaitre/Documents/packages/scikit-learn/sklearn/base.py:204:
FutureWarning: From version 0.24, get_params will raise an AttributeError 
if a parameter cannot be retrieved as an instance attribute. 
Previously it would return None. FutureWarning)
```

The reason is that we initialize `self.components_ = dictionary` meaning that `self.dictionary` does not exist which raises the warning and will error in 0.24.
Basically, `set_params` will not work as well then.

So we should probably solve this issue.

Hints from issue discussion:
No time to check this out right now, but it seems quite alarming to me, given that it implies basic functionality is broken. Thanks for the report
I have also encountered exactly the same problem. Here is a test code.
```test.py
from __future__ import print_function

import sklearn
from sklearn.decomposition import SparseCoder
from sklearn.svm import LinearSVC
from sklearn.pipeline import Pipeline
from sklearn.model_selection import GridSearchCV
from sklearn.preprocessing import Normalizer
from sklearn.datasets import load_iris

import numpy as np

# versions
print('scikit-learn ', sklearn.__version__)
print('numpy ', np.__version__)

# parameters
n_samples_dict = 5

# training samples
iris = load_iris()
# Take the first two features. We could avoid this by using a two-dim dataset
X_train = iris.data
y_train = iris.target

# dictionary
rand_dict = Normalizer(norm='l2').transform(X_train[:n_samples_dict])

# models
sp = SparseCoder(dictionary=rand_dict, transform_algorithm='threshold')
svm = LinearSVC()
pipeline = Pipeline([ ('sp', sp), ('svm', svm) ])

# grid search
grid = {'sp__transform_alpha': [0.1, 1, 10], 'svm__C': [0.1, 1, 10]}
model = GridSearchCV(estimator=pipeline, param_grid=grid, scoring='accuracy')

# check
print('Components: \n', model.estimator.named_steps['sp'].components_)
print('Sparse coder: \n', model.estimator.named_steps['sp'])
print('Sparse coder: \n', sp)
print('Sparse coder: \n', SparseCoder(dictionary=rand_dict.copy(), transform_algorithm='threshold'))

# test
X_transformed = sp.transform(X_train)
print('original: \n', X_train[5:10])
print('transformed: \n', X_transformed[5:10])

# fit
model.fit(X_train, y_train)
print(model.best_estimator_)
```

The result is the following. "Components_" in the sparse coder looks like correct, but the sparse coder instance itself seems to fail the initialisation.

```
# python test.py
scikit-learn  0.19.0
numpy  1.13.0
Components: 
 [[ 0.80377277  0.55160877  0.22064351  0.0315205 ]
 [ 0.82813287  0.50702013  0.23660939  0.03380134]
 [ 0.80533308  0.54831188  0.2227517   0.03426949]
 [ 0.80003025  0.53915082  0.26087943  0.03478392]
 [ 0.790965    0.5694948   0.2214702   0.0316386 ]]
Sparse coder: 
 SparseCoder(dictionary=None, n_jobs=1, split_sign=False,
      transform_algorithm='threshold', transform_alpha=None,
      transform_n_nonzero_coefs=None)
Sparse coder: 
 SparseCoder(dictionary=None, n_jobs=1, split_sign=False,
      transform_algorithm='threshold', transform_alpha=None,
      transform_n_nonzero_coefs=None)
Sparse coder: 
 SparseCoder(dictionary=None, n_jobs=1, split_sign=False,
      transform_algorithm='threshold', transform_alpha=None,
      transform_n_nonzero_coefs=None)
original: 
 [[ 5.4  3.9  1.7  0.4]
 [ 4.6  3.4  1.4  0.3]
 [ 5.   3.4  1.5  0.2]
 [ 4.4  2.9  1.4  0.2]
 [ 4.9  3.1  1.5  0.1]]
transformed: 
 [[ 5.87934932  5.86505251  5.87960063  5.88026013  5.88139547]
 [ 4.89118162  4.8746732   4.89092577  4.8889183   4.89427115]
 [ 5.23160303  5.22620715  5.23190722  5.23153995  5.22964031]
 [ 4.45147063  4.45215643  4.45227627  4.45585845  4.4481669 ]
 [ 4.98259107  4.98790769  4.9834534   4.98631329  4.97653151]]
Traceback (most recent call last):
  File "sparse_coder_test.py", line 54, in <module>
    model.fit(X_train, y_train)
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/model_selection/_search.py", line 624, in fit
    base_estimator = clone(self.estimator)
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/base.py", line 61, in clone
    new_object_params[name] = clone(param, safe=False)
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/base.py", line 49, in clone
    return estimator_type([clone(e, safe=safe) for e in estimator])
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/base.py", line 49, in clone
    return estimator_type([clone(e, safe=safe) for e in estimator])
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/base.py", line 62, in clone
    new_object = klass(**new_object_params)
  File "/Users/akisato/.pyenv/versions/anaconda-4.0.0/lib/python2.7/site-packages/sklearn/decomposition/dict_learning.py", line 916, in __init__
    self._set_sparse_coding_params(dictionary.shape[0],
AttributeError: 'NoneType' object has no attribute 'shape'
```


it's a big bug in SparseCoder that does in this init:

```
    def __init__(self, dictionary, transform_algorithm='omp',
                 transform_n_nonzero_coefs=None, transform_alpha=None,
                 split_sign=False, n_jobs=1):
        self._set_sparse_coding_params(dictionary.shape[0],
                                       transform_algorithm,
                                       transform_n_nonzero_coefs,
                                       transform_alpha, split_sign, n_jobs)
        self.components_ = dictionary
```

I don't know how this passed common tests...

DONT_TEST = ['SparseCoder', 'EllipticEnvelope', 'DictVectorizer', ...

On 23 August 2017 at 01:53, Alexandre Gramfort <notifications@github.com>
wrote:

> it's a big bug in SparseCoder that does in this init:
>
> ```
> def __init__(self, dictionary, transform_algorithm='omp',
> transform_n_nonzero_coefs=None, transform_alpha=None,
> split_sign=False, n_jobs=1):
> self._set_sparse_coding_params(dictionary.shape[0],
> transform_algorithm,
> transform_n_nonzero_coefs,
> transform_alpha, split_sign, n_jobs)
> self.components_ = dictionary
> ```
>
> I don't know how this passed common tests...
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/8675#issuecomment-324070510>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz65maVUDtqm4WM8L1thhpqnEG9e8Vks5savmLgaJpZM4Mvksg>
> .
>

:) :)

Fixes are welcome. Start with tests.
The question is how it passed #8022 ....
in in the init I would replace

        self.components_ = dictionary

by

        self.dictionary = dictionary

and do

        self.components_ = dictionary

in the fit

Commit message: remove one deprecated verification

Patch to review:
diff --git a/sklearn/datasets/tests/test_base.py b/sklearn/datasets/tests/test_base.py
index cb04ba5c302c3..27770a6b17287 100644
--- a/sklearn/datasets/tests/test_base.py
+++ b/sklearn/datasets/tests/test_base.py
@@ -2,7 +2,6 @@
 import shutil
 import tempfile
 import warnings
-import numpy
 from pickle import loads
 from pickle import dumps
 from functools import partial
diff --git a/sklearn/decomposition/_dict_learning.py b/sklearn/decomposition/_dict_learning.py
index d21ee6332fe79..ffaa8c0447b63 100644
--- a/sklearn/decomposition/_dict_learning.py
+++ b/sklearn/decomposition/_dict_learning.py
@@ -14,6 +14,7 @@
 from joblib import Parallel, delayed, effective_n_jobs
 
 from ..base import BaseEstimator, TransformerMixin
+from ..utils import deprecated
 from ..utils import (check_array, check_random_state, gen_even_slices,
                      gen_batches)
 from ..utils.extmath import randomized_svd, row_norms
@@ -879,16 +880,11 @@ def dict_learning_online(X, n_components=2, *, alpha=1, n_iter=100,
         return dictionary.T
 
 
-class SparseCodingMixin(TransformerMixin):
-    """Sparse coding mixin"""
-
-    def _set_sparse_coding_params(self, n_components,
-                                  transform_algorithm='omp',
-                                  transform_n_nonzero_coefs=None,
-                                  transform_alpha=None, split_sign=False,
-                                  n_jobs=None, positive_code=False,
-                                  transform_max_iter=1000):
-        self.n_components = n_components
+class _BaseSparseCoding(TransformerMixin):
+    """Base class from SparseCoder and DictionaryLearning algorithms."""
+    def __init__(self, transform_algorithm, transform_n_nonzero_coefs,
+                 transform_alpha, split_sign, n_jobs, positive_code,
+                 transform_max_iter):
         self.transform_algorithm = transform_algorithm
         self.transform_n_nonzero_coefs = transform_n_nonzero_coefs
         self.transform_alpha = transform_alpha
@@ -897,6 +893,27 @@ def _set_sparse_coding_params(self, n_components,
         self.n_jobs = n_jobs
         self.positive_code = positive_code
 
+    def _transform(self, X, dictionary):
+        """Private method allowing to accomodate both DictionaryLearning and
+        SparseCoder."""
+        X = check_array(X)
+
+        code = sparse_encode(
+            X, dictionary, algorithm=self.transform_algorithm,
+            n_nonzero_coefs=self.transform_n_nonzero_coefs,
+            alpha=self.transform_alpha, max_iter=self.transform_max_iter,
+            n_jobs=self.n_jobs, positive=self.positive_code)
+
+        if self.split_sign:
+            # feature vector is split into a positive and negative side
+            n_samples, n_features = code.shape
+            split_code = np.empty((n_samples, 2 * n_features))
+            split_code[:, :n_features] = np.maximum(code, 0)
+            split_code[:, n_features:] = -np.minimum(code, 0)
+            code = split_code
+
+        return code
+
     def transform(self, X):
         """Encode the data as a sparse combination of the dictionary atoms.
 
@@ -913,30 +930,12 @@ def transform(self, X):
         -------
         X_new : ndarray of shape (n_samples, n_components)
             Transformed data
-
         """
         check_is_fitted(self)
+        return self._transform(X, self.components_)
 
-        X = check_array(X)
-
-        code = sparse_encode(
-            X, self.components_, algorithm=self.transform_algorithm,
-            n_nonzero_coefs=self.transform_n_nonzero_coefs,
-            alpha=self.transform_alpha, max_iter=self.transform_max_iter,
-            n_jobs=self.n_jobs, positive=self.positive_code)
 
-        if self.split_sign:
-            # feature vector is split into a positive and negative side
-            n_samples, n_features = code.shape
-            split_code = np.empty((n_samples, 2 * n_features))
-            split_code[:, :n_features] = np.maximum(code, 0)
-            split_code[:, n_features:] = -np.minimum(code, 0)
-            code = split_code
-
-        return code
-
-
-class SparseCoder(SparseCodingMixin, BaseEstimator):
+class SparseCoder(_BaseSparseCoding, BaseEstimator):
     """Sparse coding
 
     Finds a sparse representation of data against a fixed, precomputed
@@ -956,7 +955,7 @@ class SparseCoder(SparseCodingMixin, BaseEstimator):
         normalized to unit norm.
 
     transform_algorithm : {'lasso_lars', 'lasso_cd', 'lars', 'omp', \
-    'threshold'}, default='omp'
+            'threshold'}, default='omp'
         Algorithm used to transform the data:
 
         - lars: uses the least angle regression method (linear_model.lars_path)
@@ -968,7 +967,7 @@ class SparseCoder(SparseCodingMixin, BaseEstimator):
         - threshold: squashes to zero all coefficients less than alpha from
           the projection ``dictionary * X'``
 
-    transform_n_nonzero_coefs : int, default=0.1*n_features
+    transform_n_nonzero_coefs : int, default=0.1 * n_features
         Number of nonzero coefficients to target in each column of the
         solution. This is only used by `algorithm='lars'` and `algorithm='omp'`
         and is overridden by `alpha` in the `omp` case.
@@ -987,7 +986,7 @@ class SparseCoder(SparseCodingMixin, BaseEstimator):
         its negative part and its positive part. This can improve the
         performance of downstream classifiers.
 
-    n_jobs : int or None, default=None
+    n_jobs : int, default=None
         Number of parallel jobs to run.
         ``None`` means 1 unless in a :obj:`joblib.parallel_backend` context.
         ``-1`` means using all processors. See :term:`Glossary <n_jobs>`
@@ -1007,7 +1006,11 @@ class SparseCoder(SparseCodingMixin, BaseEstimator):
     Attributes
     ----------
     components_ : ndarray of shape (n_components, n_features)
-        The unchanged dictionary atoms
+        The unchanged dictionary atoms.
+
+        .. deprecated:: 0.23
+           This attribute is deprecated in 0.23 and will be removed in 0.25.
+           Use `dictionary` instead.
 
     Examples
     --------
@@ -1045,15 +1048,15 @@ def __init__(self, dictionary, *, transform_algorithm='omp',
                  transform_n_nonzero_coefs=None, transform_alpha=None,
                  split_sign=False, n_jobs=None, positive_code=False,
                  transform_max_iter=1000):
-        self._set_sparse_coding_params(dictionary.shape[0],
-                                       transform_algorithm,
-                                       transform_n_nonzero_coefs,
-                                       transform_alpha, split_sign, n_jobs,
-                                       positive_code, transform_max_iter)
-        self.components_ = dictionary
+        super().__init__(
+            transform_algorithm, transform_n_nonzero_coefs,
+            transform_alpha, split_sign, n_jobs, positive_code,
+            transform_max_iter
+        )
+        self.dictionary = dictionary
 
     def fit(self, X, y=None):
-        """Do nothing and return the estimator unchanged
+        """Do nothing and return the estimator unchanged.
 
         This method is just there to implement the usual API and hence
         work in pipelines.
@@ -1067,16 +1070,48 @@ def fit(self, X, y=None):
         Returns
         -------
         self : object
-            Returns the object itself
         """
         return self
 
+    @deprecated("The attribute 'components_' is deprecated "  # type: ignore
+                "in 0.23 and will be removed in 0.25. Use the "
+                "'dictionary' instead.")
+    @property
+    def components_(self):
+        return self.dictionary
+
+    def transform(self, X, y=None):
+        """Encode the data as a sparse combination of the dictionary atoms.
+
+        Coding method is determined by the object parameter
+        `transform_algorithm`.
+
+        Parameters
+        ----------
+        X : array of shape (n_samples, n_features)
+            Test data to be transformed, must have the same number of
+            features as the data used to train the model.
+
+        Returns
+        -------
+        X_new : array of shape (n_samples, n_components)
+            Transformed data
+        """
+        return super()._transform(X, self.dictionary)
+
+    def _more_tags(self):
+        return {"requires_fit": False}
+
+    @property
+    def n_components_(self):
+        return self.dictionary.shape[0]
+
     @property
     def n_features_in_(self):
-        return self.components_.shape[1]
+        return self.dictionary.shape[1]
 
 
-class DictionaryLearning(SparseCodingMixin, BaseEstimator):
+class DictionaryLearning(_BaseSparseCoding, BaseEstimator):
     """Dictionary learning
 
     Finds a dictionary (a set of atoms) that can best be used to represent data
@@ -1247,10 +1282,12 @@ def __init__(self, n_components=None, *, alpha=1, max_iter=1000, tol=1e-8,
                  split_sign=False, random_state=None, positive_code=False,
                  positive_dict=False, transform_max_iter=1000):
 
-        self._set_sparse_coding_params(n_components, transform_algorithm,
-                                       transform_n_nonzero_coefs,
-                                       transform_alpha, split_sign, n_jobs,
-                                       positive_code, transform_max_iter)
+        super().__init__(
+            transform_algorithm, transform_n_nonzero_coefs,
+            transform_alpha, split_sign, n_jobs, positive_code,
+            transform_max_iter
+        )
+        self.n_components = n_components
         self.alpha = alpha
         self.max_iter = max_iter
         self.tol = tol
@@ -1302,7 +1339,7 @@ def fit(self, X, y=None):
         return self
 
 
-class MiniBatchDictionaryLearning(SparseCodingMixin, BaseEstimator):
+class MiniBatchDictionaryLearning(_BaseSparseCoding, BaseEstimator):
     """Mini-batch dictionary learning
 
     Finds a dictionary (a set of atoms) that can best be used to represent data
@@ -1480,10 +1517,11 @@ def __init__(self, n_components=None, *, alpha=1, n_iter=1000,
                  positive_code=False, positive_dict=False,
                  transform_max_iter=1000):
 
-        self._set_sparse_coding_params(n_components, transform_algorithm,
-                                       transform_n_nonzero_coefs,
-                                       transform_alpha, split_sign, n_jobs,
-                                       positive_code, transform_max_iter)
+        super().__init__(
+            transform_algorithm, transform_n_nonzero_coefs, transform_alpha,
+            split_sign, n_jobs, positive_code, transform_max_iter
+        )
+        self.n_components = n_components
         self.alpha = alpha
         self.n_iter = n_iter
         self.fit_algorithm = fit_algorithm
diff --git a/sklearn/decomposition/tests/test_dict_learning.py b/sklearn/decomposition/tests/test_dict_learning.py
index 547dc175ee26d..be836322d6f4b 100644
--- a/sklearn/decomposition/tests/test_dict_learning.py
+++ b/sklearn/decomposition/tests/test_dict_learning.py
@@ -3,6 +3,8 @@
 import numpy as np
 import itertools
 
+from sklearn.base import clone
+
 from sklearn.exceptions import ConvergenceWarning
 
 from sklearn.utils import check_array
@@ -498,6 +500,19 @@ def test_sparse_coder_estimator():
     assert np.sqrt(np.sum((np.dot(code, V) - X) ** 2)) < 0.1
 
 
+def test_sparse_coder_estimator_clone():
+    n_components = 12
+    rng = np.random.RandomState(0)
+    V = rng.randn(n_components, n_features)  # random init
+    V /= np.sum(V ** 2, axis=1)[:, np.newaxis]
+    code = SparseCoder(dictionary=V, transform_algorithm='lasso_lars',
+                       transform_alpha=0.001)
+    cloned = clone(code)
+    assert cloned.dictionary.tolist() == code.dictionary.tolist()
+    assert cloned.n_components_ == code.n_components_
+    assert cloned.n_features_in_ == code.n_features_in_
+
+
 def test_sparse_coder_parallel_mmap():
     # Non-regression test for:
     # https://github.com/scikit-learn/scikit-learn/issues/5956
@@ -517,6 +532,18 @@ def test_sparse_coder_parallel_mmap():
     sc.fit_transform(data)
 
 
+# TODO: remove in 0.25
+def test_sparse_coder_deprecation():
+    # check that we raise a deprecation warning when accessing `components_`
+    rng = np.random.RandomState(777)
+    n_components, n_features = 40, 64
+    init_dict = rng.rand(n_components, n_features)
+    sc = SparseCoder(init_dict)
+
+    with pytest.warns(FutureWarning, match="'components_' is deprecated"):
+        sc.components_
+
+
 def test_sparse_coder_n_features_in():
     d = np.array([[1, 2, 3], [1, 2, 3]])
     sc = SparseCoder(d)


Code review comments:
diff_hunk: @@ -498,6 +500,19 @@ def test_sparse_coder_estimator():
     assert np.sqrt(np.sum((np.dot(code, V) - X) ** 2)) < 0.1
 
 
+def test_sparse_coder_estimator_clone():
+    n_components = 12
+    rng = np.random.RandomState(0)
+    V = rng.randn(n_components, n_features)  # random init
+    V /= np.sum(V ** 2, axis=1)[:, np.newaxis]
+    code = SparseCoder(dictionary=V, transform_algorithm='lasso_lars',
+                       transform_alpha=0.001)
+    cloned = clone(code)
+    assert cloned.dictionary.tolist() == code.dictionary.tolist()
path: sklearn/decomposition/tests/test_dict_learning.py
text: @user1:
Since we are cloning, I assume that we want to check that we don't have the same `id`?

@author:
Fixed.

@user2:
I assume @user1 meant:

```python
assert id(cloned.dictionary) != id(code.dictionary)
```

which should be the case when using `clone(est, safe=True)` which is the default. For instance it is the case for KMeans:


```python
>>> import numpy as np                                                                           
>>> from sklearn.base import clone                                                               
>>> from sklearn.cluster import KMeans                                                           
>>> init = np.random.randn(100, 10)                                                              
>>> km = KMeans(n_clusters=init.shape[0], init=init)                                             
>>> cloned_km = clone(km)                                                                        
>>> id(cloned_km.init) != id(km.init)                                                            
True
```

@user2:
Instead of tolist, we should probably use

```python
assert_allclose(cloned.dictionary, code.dictionary)
```

`numpy.testing.assert_allclose` is the recommended way to compare that two numpy array with floating point values are the same.

diff_hunk: @@ -517,6 +532,18 @@ def test_sparse_coder_parallel_mmap():
     sc.fit_transform(data)
 
 
+# TODO: remove in 0.25
path: sklearn/decomposition/tests/test_dict_learning.py
text: @user1:
```suggestion
# TODO: remove in 0.26
```

diff_hunk: @@ -1067,16 +1070,48 @@ def fit(self, X, y=None):
         Returns
         -------
         self : object
-            Returns the object itself
         """
         return self
 
+    @deprecated("The attribute 'components_' is deprecated "  # type: ignore
+                "in 0.23 and will be removed in 0.25. Use the "
path: sklearn/decomposition/_dict_learning.py
text: @user1:
```suggestion
                "in 0.24 and will be removed in 0.26. Use the "
```