[MRG] value error when distance matrix is not square and affinity=precomputed #16257

simonamaggio · 2020-01-28T16:32:20Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Added a check in AgglomerativeClustering, to make sure the input matrix of fit() is square, when affinity=precomputed.
This raises a more clear error when a non square matrix is used as input (instead of out of bounds IndexError).

Any other comments?

…shape

jnothman · 2020-01-28T22:34:04Z

sklearn/cluster/_agglomerative.py

@@ -455,6 +455,11 @@ def linkage_tree(X, connectivity=None, n_clusters=None, linkage='complete',
            # data, provide as first argument an ndarray of the shape returned
            # by pdist: it is a flat array containing the upper triangular of
            # the distance matrix.
+            if len(X.shape) != 2 or X.shape[0] != X.shape[1]:
+                raise ValueError(
+                    'Matrix should be square, as returned by pdist. '


pairwise_distances would be more familiar to our users than pdist

Ok, thanks. Let me change the Error Message.

…e_distances. Update test accordingly.

lesteve · 2020-01-29T14:41:46Z

sklearn/cluster/_agglomerative.py

+            # containing the upper triangular of the distance matrix.
+            if len(X.shape) != 2 or X.shape[0] != X.shape[1]:
+                raise ValueError(
+                    'Matrix should be square, '


I am a bit confused here. I have to say I am not sure what shape X should have when affinity == 'precomputed'.

The thing is at the moment, the error message says that X should be square and the comment above says that X should be flat ...

@jeremiedbb thinks that the matrix should be square because of the code below:

i, j = np.triu_indices(X.shape[0], k=1) X = X[i, j] # now X is a flat array containing the upper triangular of the distance matrix

So the comment needs to be changed.

Rather than

provide as first argument an ndarray of the shape returned by pdist: it is a flat array containing the upper triangular of by sklearn.metrics.pairwise_distances: it is a flat array

say something like:

X should be a distance matrix as returned by sklearn.metrics.pairwise_distances

I removed the comment. The input matrix should be squared. And for sure it shouldn't be flat, as there is another check somewhere else asking the user to reshape the potential 1D input as a 2D matrix (n,1).

jeremiedbb

Thanks @simonamaggio. A few comments below.

sklearn/cluster/tests/test_hierarchical.py

jeremiedbb · 2020-01-30T09:43:49Z

sklearn/cluster/_agglomerative.py

+                    'Matrix should be square, '
+                    'as returned by pairwise_distances. '
+                    'Found dimensionality %s' % str(X.shape)


I would not say as return by pairwise_distances, because pairwise distances can return non square matrix (if called on X and Y with different shapes). I would just say

'Matrix should be squared. Found matrix of shape {}'.format(X.shape)

Done. Just added 'Distance matrix' instead of 'Matrix' to be more specific.

Please use the .format syntax.

jeremiedbb

Besides my last comment, lgtm !

jnothman

Otherwise lgtm

jnothman · 2020-01-30T21:12:11Z

sklearn/cluster/_agglomerative.py

+            if len(X.shape) != 2 or X.shape[0] != X.shape[1]:
+                raise ValueError(
+                    'Distance matrix should be square, '
+                    'Found dimensionality %s' % str(X.shape)


Suggested change

'Found dimensionality %s' % str(X.shape)

'Got shape %s' % str(X.shape)

Changed it.

glemaitre

We will need an entry in the whats new as well.

sklearn/cluster/_agglomerative.py

sklearn/cluster/tests/test_hierarchical.py

…ginning of fit()

jeremiedbb · 2020-02-10T13:22:46Z

@simonamaggio could you please add an entry in the what's new (scikit-learn/doc/whats_new/v0.23.rst) ? you can tag it |Fix|.

doc/whats_new/v0.23.rst

Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>

glemaitre

Sorry I did not see the typo the first time.
LGTM otherwise

glemaitre · 2020-02-17T18:36:55Z

sklearn/cluster/_agglomerative.py

+            # by sklearn.metrics.pairwise_distances.
+            if X.shape[0] != X.shape[1]:
+                raise ValueError(
+                    'Distance matrix should be square, '


Suggested change

'Distance matrix should be square, '

'Distance matrix should be squared, '

Are you sure? I think it's "square matrix", not "squared matrix". This doesn't seem right, but if you insist, I'll change it.

You are right.

glemaitre · 2020-02-17T18:37:17Z

sklearn/cluster/_agglomerative.py

+            if X.shape[0] != X.shape[1]:
+                raise ValueError(
+                    'Distance matrix should be square, '
+                    'Got matrix of shape {}'.format(X.shape)


Suggested change

'Got matrix of shape {}'.format(X.shape)

f'Got matrix of shape {X.shape}.'

OK for this.

glemaitre · 2020-02-17T18:37:30Z

sklearn/cluster/tests/test_hierarchical.py

+    # and a non square matrix is passed (PR #16257).
+    rng = np.random.RandomState(0)
+    X = rng.rand(5, 3)
+    with pytest.raises(ValueError, match="Distance matrix should be square, "):


Suggested change

with pytest.raises(ValueError, match="Distance matrix should be square, "):

with pytest.raises(ValueError, match="Distance matrix should be squared, "):

Are you sure? I think it's "square matrix", not "squared matrix". This doesn't seem right, but if you insist, I'll change it.

glemaitre · 2020-02-17T22:03:22Z

@simonamaggio Thank you for finishing the PR. Do not hesitate to take another one :)

…=precomputed (scikit-learn#16257)

Check fit() input matrix is square when affinity==precomputed

0920d3a

simonamaggio changed the title ~~[MRG]~~ [MRG] value error when distance matrix is not square and affinity=precomputed Jan 28, 2020

jeremiedbb added the Sprint label Jan 28, 2020

Simona Maggio added 2 commits January 28, 2020 22:02

add test for ValueError when precomputed distance matrix has invalid …

c8dcfd8

…shape

Fix Linting issue

80e6e60

jnothman reviewed Jan 28, 2020

View reviewed changes

Changed comment and error message, replacing scipy pdist with pairwis…

508295a

…e_distances. Update test accordingly.

lesteve reviewed Jan 29, 2020

View reviewed changes

Remove ambiguous and wrong comment about an input flat array

0755689

jeremiedbb reviewed Jan 30, 2020

View reviewed changes

Simona Maggio added 2 commits January 30, 2020 11:11

More compact code and more explicit comments and error messages

7a6a54f

bugfix: same message in test and main code

9b8f978

jeremiedbb approved these changes Jan 30, 2020

View reviewed changes

jnothman reviewed Jan 30, 2020

View reviewed changes

glemaitre requested changes Jan 31, 2020

View reviewed changes

sklearn/cluster/_agglomerative.py Outdated Show resolved Hide resolved

sklearn/cluster/tests/test_hierarchical.py Show resolved Hide resolved

Simona Maggio added 2 commits February 3, 2020 13:35

use format() and clarify error message

97c8dbb

Remove check array size in condition, as it is already done in the be…

86b04bf

…ginning of fit()

jeremiedbb mentioned this pull request Feb 6, 2020

value error when "precomputed" distance matrix is not square. #4901

Closed

Simona Maggio and others added 2 commits February 17, 2020 09:19

Add entry in whats_new/v0.23.rst

42a341d

Merge branch 'master' into check_precomputed_shape_agglo_clustering

84202d8

glemaitre reviewed Feb 17, 2020

View reviewed changes

doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved

Update doc/whats_new/v0.23.rst

1f37c17

Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>

glemaitre approved these changes Feb 17, 2020

View reviewed changes

simonamaggio closed this Feb 17, 2020

simonamaggio reopened this Feb 17, 2020

Simona Maggio added 3 commits February 17, 2020 21:34

two dots missing

2109719

fix format in print message

946950a

merge with local file: addind extra points

7365067

glemaitre merged commit 06c71ec into scikit-learn:master Feb 17, 2020

simonamaggio deleted the check_precomputed_shape_agglo_clustering branch February 18, 2020 10:55

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020

BUG raise value error when distance matrix is not square and affinity…

cdfaffc

…=precomputed (scikit-learn#16257)

panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020

BUG raise value error when distance matrix is not square and affinity…

36393a8

…=precomputed (scikit-learn#16257)

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

BUG raise value error when distance matrix is not square and affinity…

676e397

…=precomputed (scikit-learn#16257)

	'Found dimensionality %s' % str(X.shape)
	'Got shape %s' % str(X.shape)

	'Distance matrix should be square, '
	'Distance matrix should be squared, '

	'Got matrix of shape {}'.format(X.shape)
	f'Got matrix of shape {X.shape}.'

	with pytest.raises(ValueError, match="Distance matrix should be square, "):
	with pytest.raises(ValueError, match="Distance matrix should be squared, "):

Uh oh!

[MRG] value error when distance matrix is not square and affinity=precomputed #16257

[MRG] value error when distance matrix is not square and affinity=precomputed #16257

Uh oh!

Conversation

simonamaggio commented Jan 28, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

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!

Choose a reason for hiding this comment

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman left a comment

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!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeremiedbb commented Feb 10, 2020

Uh oh!

Uh oh!

glemaitre left a comment

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!

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!

glemaitre commented Feb 17, 2020

Uh oh!

Uh oh!