[MRG] FIX sklearn.cluster.spectral_clustering fails with `np.matrix` input (#10993) #11070

sinanh · 2018-05-05T10:36:34Z

Reference Issues/PRs

Fixes #10993

What does this implement/fix? Explain your changes.

It fixes a small bug in spectral_embedding while finding the neighbors when the input array is a np.matrix.

Any other comments?

jnothman · 2018-05-06T01:36:48Z

Not changing estimator_checks?

sinanh · 2018-05-06T07:53:33Z

At first I changed estimator_checks, then I noticed that was not necessary. Because after some testing, I noticed that all the clustering algorithms (e.g. KMeans, AgglomerativeClustering, MeanShift, DBSCAN etc.) work with np.matrix smoothly. Thus, I think the mentioned problem in this issue was limited to Spectral Clustering.

jnothman · 2018-05-06T22:09:44Z

interesting. I would still be keen on a common test to ensure matrix works across the board, for instance. Your test whether clusterers work was unlikely to check that attributes stored are arrays and not matrices, so I also have my doubts about how certain you are that they completely worked...

glemaitre · 2018-05-07T09:53:51Z

Spectral clustering will call the check_array while spectral_clustering will not. check_array will convert the matrix to array. Is it meaning that we should have a second sanity check in the function themselves instead of only the class.

jnothman

Otherwise this LGTM.

Though we should really have a generic test for consistency between class and function versions of clusterers that makes this check, instead of ad-hoc tests.

jnothman · 2018-05-07T22:59:40Z

sklearn/manifold/spectral_embedding_.py

@@ -60,6 +60,8 @@ def _graph_connected_component(graph, node_id):
        for i in indices:
            if sparse.issparse(graph):
                neighbors = graph[i].toarray().ravel()
+            elif isinstance(graph, np.matrix):


Can we just convert and store as graph at the top of the function?

jnothman · 2018-05-07T23:03:06Z

we would not want an expensive check in the function version. those should be low overheard to enable reuse in other code.

amueller · 2018-05-21T23:51:12Z

I thought we didn't support np.matrix, but I guess if check_array accepts it then it's fine?

rth · 2019-06-16T13:07:07Z

sklearn/manifold/spectral_embedding_.py

@@ -44,6 +44,8 @@ def _graph_connected_component(graph, node_id):
        node
    """
    n_node = graph.shape[0]
+    graph = sparse.csc_matrix(graph)


This does not look right -- it is converted to CSR on the following line.

Thanks, I think you are right.

To be compliant with jnothman's suggestion on May 8th about converting and storing it as a graph at the top, I suggest the following actions:

Change line 48 to: graph = sparse.csr_matrix(graph)
Delete lines 49-51

I'm a bit confused about this. @jnothman?

To be compliant with jnothman's suggestion on May 8th about converting and storing it as a graph at the top, I suggest the following actions:

What I said was "Can we just convert and store as graph at the top of the function?"

What I meant was "Can we just convert the matrix to an array and assign it back to the `graph variable at the top of the function?"

This does not look right -- it is converted to CSR on the following line.

Actually, since it must be a symmetric matrix, its row and column matrix representations are equivalent. Thus it was passing the tests.

converting the graph to sparse.csc_matrix was passing the tests, because it must be a symmetric matrix. So its column matrix (csc) and row matrix (csr) are equivalent.

cmarmo · 2020-08-25T09:15:26Z

Hi @sinanh , thanks for your patience! Are you still interested in working on this PR? If so do you mind fixing conflicts? Thanks!

sinanh · 2020-08-26T10:48:03Z

Hi @sinanh , thanks for your patience! Are you still interested in working on this PR? If so do you mind fixing conflicts? Thanks!

I fixed the conflicts .

cmarmo · 2020-08-26T10:57:15Z

Thanks @sinanh! @jnothman , @glemaitre maybe you might want to have a look? Thanks!

jnothman · 2020-08-26T20:49:43Z

sklearn/manifold/_spectral_embedding.py

@@ -44,6 +44,9 @@ def _graph_connected_component(graph, node_id):
        node.
    """
    n_node = graph.shape[0]
+    if isinstance(graph, np.matrix):


instead, can we just pass np.asarray(graph) to _graph_connected_component, instead of passing graph unchanged?

@jnothman OK, I did that. Also I noticed since numpy 1.15, it is advised not to use np.matrix. The following is written in the manual:

"It is no longer recommended to use this class, even for linear algebra. Instead use regular arrays. The class may be removed in the future."

cmarmo · 2020-09-04T19:10:45Z

Hi @sinanh, I guess you closed as you are tired of waiting? I can't do more than relabeling as "waiting for review"... but I wonder if you could reconsider...thanks for your work, anyway.

sinanh · 2020-09-04T19:16:37Z

Hi @sinanh, I guess you closed as you are tired of waiting? I can't do more than relabeling as "waiting for review"... but I wonder if you could reconsider...thanks for your work, anyway.

@cmarmo thanks, the thing is from numpy 1.15 it is advised not to use np.matrix, so this issue is not an issue anymore I think. I guess both the issue and PR can be closed.

cmarmo · 2020-09-04T19:25:40Z

@sinanh, understood: thanks for clarifying. If you are still interested in working on scikit-learn, feel free to pick one among the "help wanted" issues. Thanks for your time.

sinanh and others added 4 commits May 5, 2018 13:04

fixes 10993

0700ed8

Merge remote-tracking branch 'upstream/master' into fix_issue_10993

7aefc5c

Update test_spectral.py

6f386a6

PEP8 fixes

73a99ab

jnothman reviewed May 7, 2018

View reviewed changes

ensure arrays and matrices are converted to a graph

d4830e0

rth reviewed Jun 16, 2019

View reviewed changes

glemaitre self-requested a review June 20, 2019 12:38

sinanh and others added 8 commits August 17, 2019 17:21

Update spectral_embedding_.py

d767112

converting the graph to sparse.csc_matrix was passing the tests, because it must be a symmetric matrix. So its column matrix (csc) and row matrix (csr) are equivalent.

for merge

3e9ec4b

fixes 10993

9df2541

PEP8 fixes

0203732

ensure arrays and matrices are converted to a graph

034e37c

Update spectral_embedding_.py

a480225

converting the graph to sparse.csc_matrix was passing the tests, because it must be a symmetric matrix. So its column matrix (csc) and row matrix (csr) are equivalent.

fix

5c49ea5

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn

fabd17d

sinanh force-pushed the fix_issue_10993 branch 3 times, most recently from a508eb5 to d4830e0 Compare August 20, 2019 13:17

github-actions bot added module:cluster module:manifold labels Mar 2, 2020

cmarmo added the Waiting for Reviewer label Aug 26, 2020

sinanh force-pushed the fix_issue_10993 branch from 292c7e4 to fabd17d Compare August 26, 2020 09:02

squash commits

2bdf8ee

sinanh force-pushed the fix_issue_10993 branch from 78d236c to 2bdf8ee Compare August 26, 2020 09:27

remove column_transformer

5a83d07

jnothman reviewed Aug 26, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label Aug 27, 2020

sinanh added 2 commits August 27, 2020 19:33

Merge remote-tracking branch 'upstream/master' into fix_issue_10993

4dd3c67

pass as ndarray

48d2b29

sinanh closed this Sep 4, 2020

cmarmo added the Waiting for Reviewer label Sep 4, 2020

cmarmo removed the Waiting for Reviewer label Sep 4, 2020

cmarmo mentioned this pull request Sep 4, 2020

sklearn.cluster.spectral_clustering fails with np.matrix input #10993

Closed

sinanh deleted the fix_issue_10993 branch September 5, 2020 07:22

yoh1496 mentioned this pull request May 28, 2021

DOC Add note about that numpy.matrix is not supported (Fixes: #10993) #20157

Closed

Uh oh!

[MRG] FIX sklearn.cluster.spectral_clustering fails with np.matrix input (#10993) #11070

[MRG] FIX sklearn.cluster.spectral_clustering fails with np.matrix input (#10993) #11070

Uh oh!

Conversation

sinanh commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman commented May 6, 2018

Uh oh!

sinanh commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented May 6, 2018 via email

Uh oh!

glemaitre commented May 7, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented May 7, 2018 via email

Uh oh!

amueller commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmarmo commented Aug 25, 2020

Uh oh!

sinanh commented Aug 26, 2020

Uh oh!

cmarmo commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmarmo commented Sep 4, 2020

Uh oh!

sinanh commented Sep 4, 2020

Uh oh!

cmarmo commented Sep 4, 2020

Uh oh!

Uh oh!

[MRG] FIX sklearn.cluster.spectral_clustering fails with `np.matrix` input (#10993) #11070

[MRG] FIX sklearn.cluster.spectral_clustering fails with `np.matrix` input (#10993) #11070

sinanh commented May 5, 2018 •

edited

Loading

sinanh commented May 6, 2018 •

edited

Loading