Repository: scikit-learn/scikit-learn
Pull request: [MRG] Fix n_components kwarg missing in SpectralClustering. See #13698
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
Fixes #13698 
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->


#### What does this implement/fix? Explain your changes.
The `SpectralClustering` class does not have an instance variable `n_components`; that's why `n_components` cannot be accessed by an instance of `SpectralClustering` class. My temporary fix is to add `n_components` as an instance variable of `SpectralClustering` class, and add it as an argument of `spectral_clustering` in `fit`. 
If this is a reasonable fix of the problem, I will then add a test function `def test_n_components` in `test_spectral.py`. Otherwise please let me know what is the best way to fix this.

#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
n_components kwarg missing in SpectralClustering
The `n_components` kwarg defined in the `spectral_clustering` function allow the user to choose how many eigenvalues/eigenvectors should be used in the classification.
However this kwarg cannot be accessed/modified when using `SpectralClustering` class, and is set to default (`n_components` = `nb_clusters`). Since the `SpectralClustering` class calls the spectral_clustering function, I guess the user should be able to access this kwarg ?

Hints from issue discussion:
I don't know the history of this. Maybe someone wants to go digging to find out whether this was a reasoned choice by the developers
I looked at the code, and obviously `n_components` is not a instance variable of the `SpectralClustering` class, and when an instance of `SpectralClustering` class calls fit, `n_components` is not passed into `spectral_clustering` method. So should the fix of this problem be adding a `n_components` instance variable to the `SpectralClustering` class? Or is there any other concern for not doing so? @adrinjalali

If adding a `n_components` instance variable is a reasonable fix, I would like to work on this. Thanks

Commit message: fix doctest failure

Patch to review:
diff --git a/sklearn/cluster/spectral.py b/sklearn/cluster/spectral.py
index 75757ae907717..c48117b87f1b9 100644
--- a/sklearn/cluster/spectral.py
+++ b/sklearn/cluster/spectral.py
@@ -307,6 +307,9 @@ class SpectralClustering(BaseEstimator, ClusterMixin):
         to be installed. It can be faster on very large, sparse problems,
         but may also lead to instabilities
 
+    n_components : integer, optional, default is n_clusters
+        Number of eigen vectors to use for the spectral embedding
+
     random_state : int, RandomState instance or None (default)
         A pseudo random number generator used for the initialization of the
         lobpcg eigen vectors decomposition when eigen_solver == 'amg' and by
@@ -383,12 +386,12 @@ class SpectralClustering(BaseEstimator, ClusterMixin):
     ...         assign_labels="discretize",
     ...         random_state=0).fit(X)
     >>> clustering.labels_
-    array([1, 1, 1, 0, 0, 0])
+    array([1, 1, 1, 0, 0, 0], dtype=int64)
     >>> clustering # doctest: +NORMALIZE_WHITESPACE
     SpectralClustering(affinity='rbf', assign_labels='discretize', coef0=1,
               degree=3, eigen_solver=None, eigen_tol=0.0, gamma=1.0,
-              kernel_params=None, n_clusters=2, n_init=10, n_jobs=None,
-              n_neighbors=10, random_state=0)
+              kernel_params=None, n_clusters=2, n_components=2, n_init=10,
+              n_jobs=None, n_neighbors=10, random_state=0)
 
     Notes
     -----
@@ -425,12 +428,14 @@ class SpectralClustering(BaseEstimator, ClusterMixin):
       https://www1.icsi.berkeley.edu/~stellayu/publication/doc/2003kwayICCV.pdf
     """
 
-    def __init__(self, n_clusters=8, eigen_solver=None, random_state=None,
-                 n_init=10, gamma=1., affinity='rbf', n_neighbors=10,
-                 eigen_tol=0.0, assign_labels='kmeans', degree=3, coef0=1,
-                 kernel_params=None, n_jobs=None):
+    def __init__(self, n_clusters=8, eigen_solver=None, n_components=None,
+                 random_state=None, n_init=10, gamma=1., affinity='rbf',
+                 n_neighbors=10, eigen_tol=0.0, assign_labels='kmeans',
+                 degree=3, coef0=1, kernel_params=None, n_jobs=None):
         self.n_clusters = n_clusters
         self.eigen_solver = eigen_solver
+        self.n_components = n_clusters if n_components \
+            is None else n_components
         self.random_state = random_state
         self.n_init = n_init
         self.gamma = gamma
@@ -486,6 +491,7 @@ def fit(self, X, y=None):
         random_state = check_random_state(self.random_state)
         self.labels_ = spectral_clustering(self.affinity_matrix_,
                                            n_clusters=self.n_clusters,
+                                           n_components=self.n_components,
                                            eigen_solver=self.eigen_solver,
                                            random_state=random_state,
                                            n_init=self.n_init,
diff --git a/sklearn/cluster/tests/test_spectral.py b/sklearn/cluster/tests/test_spectral.py
index 0c220e7615e67..67cf9cfa73da3 100644
--- a/sklearn/cluster/tests/test_spectral.py
+++ b/sklearn/cluster/tests/test_spectral.py
@@ -108,8 +108,7 @@ def test_affinities():
     # a dataset that yields a stable eigen decomposition both when built
     # on OSX and Linux
     X, y = make_blobs(n_samples=20, random_state=0,
-                      centers=[[1, 1], [-1, -1]], cluster_std=0.01
-                     )
+                      centers=[[1, 1], [-1, -1]], cluster_std=0.01)
     # nearest neighbors affinity
     sp = SpectralClustering(n_clusters=2, affinity='nearest_neighbors',
                             random_state=0)
@@ -205,3 +204,16 @@ def test_spectral_clustering_with_arpack_amg_solvers():
         assert_raises(
             ValueError, spectral_clustering,
             graph, n_clusters=2, eigen_solver='amg', random_state=0)
+
+
+def test_n_components():
+    # no value passed to n_components
+    sp = SpectralClustering(n_clusters=4, affinity="nearest_neighbors",
+                            random_state=0)
+    assert sp.n_components == sp.n_clusters
+
+    # set n_components value to be 2
+    n_comp = 2
+    sp = SpectralClustering(n_components=n_comp, affinity="nearest_neighbors",
+                            random_state=0)
+    assert sp.n_components == n_comp


Code review comments:
diff_hunk: @@ -425,12 +428,14 @@ class SpectralClustering(BaseEstimator, ClusterMixin):
       https://www1.icsi.berkeley.edu/~stellayu/publication/doc/2003kwayICCV.pdf
     """
 
-    def __init__(self, n_clusters=8, eigen_solver=None, random_state=None,
-                 n_init=10, gamma=1., affinity='rbf', n_neighbors=10,
-                 eigen_tol=0.0, assign_labels='kmeans', degree=3, coef0=1,
-                 kernel_params=None, n_jobs=None):
+    def __init__(self, n_clusters=8, eigen_solver=None, n_components=None,
+                 random_state=None, n_init=10, gamma=1., affinity='rbf',
+                 n_neighbors=10, eigen_tol=0.0, assign_labels='kmeans',
+                 degree=3, coef0=1, kernel_params=None, n_jobs=None):
         self.n_clusters = n_clusters
         self.eigen_solver = eigen_solver
+        self.n_components = n_clusters if n_components \
path: sklearn/cluster/spectral.py
text: @user1:
Set `self.n_components = n_components` and pass `self.n_components` directly to `spectral_clustering` without modifying any of the parameters in `__init__`. `spectral_clustering` already handles the case when `n_components` is defined:

https://github.com/scikit-learn/scikit-learn/blob/8d3b4ff3eec890396a3d7a806bbe944f55a89cb4/sklearn/cluster/spectral.py#L256
 

@author:
Nice idea! thanks for help