[MRG+2] algorithm='auto' should always work for nearest neighbors (continuation) by herilalaina · Pull Request #9145 · scikit-learn/scikit-learn

herilalaina · 2017-06-17T00:33:21Z

Reference Issue

Fixes #4931, continuation of #5596

What does this implement/fix? Explain your changes.

Implement test for metric in ['mahalanobis', 'wminkowski', 'seuclidean']

Any other comments?

Should we warn the user when the algorithm is set into brute? (instead of auto)

herilalaina · 2017-06-17T00:35:54Z

sklearn/neighbors/tests/test_neighbors.py

+        if metric not in _metrics:
+            nn = neighbors.NearestNeighbors(n_neighbors=3, algorithm='auto',
+                                            metric=metric).fit(Xcsr)
+            if metric != "precomputed":


Is that normal that I can't call kneighbors method on sparse matrix when metric='precomputed'

That case is not implemented: it would need to be treated a bit differently to the precomputed dense array case; and it doesn't really make sense. An array containing distances which is mostly 0s is pretty useless for nearest neighbors. (Unless we're admitting negative distances, or having a different interpretation of 0s, both of which may be useful.)

But I don't understand why precomputed should be in VALID_METRICS_SPARSE and hence why it's relevant tot his code path.

jnothman

otherwise LGTM

jnothman · 2017-06-17T11:08:10Z

sklearn/neighbors/tests/test_neighbors.py

+        if metric not in _metrics:
+            nn = neighbors.NearestNeighbors(n_neighbors=3, algorithm='auto',
+                                            metric=metric).fit(Xcsr)
+            if metric != "precomputed":


That case is not implemented: it would need to be treated a bit differently to the precomputed dense array case; and it doesn't really make sense. An array containing distances which is mostly 0s is pretty useless for nearest neighbors. (Unless we're admitting negative distances, or having a different interpretation of 0s, both of which may be useful.)

jnothman · 2017-06-17T11:10:17Z

sklearn/neighbors/tests/test_neighbors.py

+        if metric not in _metrics:
+            nn = neighbors.NearestNeighbors(n_neighbors=3, algorithm='auto',
+                                            metric=metric).fit(Xcsr)
+            if metric != "precomputed":


But I don't understand why precomputed should be in VALID_METRICS_SPARSE and hence why it's relevant tot his code path.

jnothman · 2017-06-17T11:13:34Z

sklearn/neighbors/tests/test_neighbors.py

    assert_array_almost_equal(dist1, dist2)


+def test_algo_auto_metrics():


a better name or comment would be appreciated

jnothman · 2017-06-17T11:14:18Z

doc/modules/neighbors.rst

+``'effective_metric_'`` is in the ``'VALID_METRICS'`` list of
+``'ball_tree'``. It selects ``'brute'`` if :math:`k < N/2` and the
+``'effective_metric_'`` is not in the ``'VALID_METRICS'`` list of
+``'kd_tree'`` or ``'ball_tree'``. It selects ``'brute'`` if :math:`k >= N/2`. This choice is based on the assumption that the number of query points is at least the


could you please keep this under 80 chars

herilalaina · 2017-06-17T12:46:52Z

Thanks for the review. The change has been done!

jnothman · 2017-06-17T12:54:38Z

sklearn/neighbors/tests/test_neighbors.py

    assert_array_almost_equal(dist1, dist2)


+def test_unsupported_metric_for_auto():


I don't know what you mean by unsupported. It's clearly not unsupported if you're testing it works.

amueller · 2017-06-19T15:54:49Z

sklearn/neighbors/tests/test_neighbors.py

+    X = rng.rand(12, 12)
+    Xcsr = csr_matrix(X)
+
+    # Metric which don't required any additional parameter


I find the comment a bit misleading. I would rename the variable and say only test metrics that don't require additional arguments.
And maybe assert that some strange metric is in VALID_METRICS['brute'].

Checking that the test is non-empty, and more didactic variable name

amueller · 2017-06-19T16:03:20Z

LGTM, I made minor minor changes to fix my nitpicks, merge on green.

herilalaina · 2017-06-19T16:30:57Z

Thank you!

…ntinuation) (scikit-learn#9145) * Merge neighbors.rst * issue scikit-learn#4931 * Improve test implementation * Update base.py * Remove unused import * Customize test for precomputed metric * Change test function name * rename _metrics to require_params, add set assert Checking that the test is non-empty, and more didactic variable name * Remove blank line

herilalaina and others added 4 commits June 17, 2017 00:01

Merge neighbors.rst

af53a35

issue scikit-learn#4931

792f0f9

Improve test implementation

3dba889

Update base.py

561624b

herilalaina commented Jun 17, 2017

View reviewed changes

Remove unused import

6f686b2

herilalaina changed the title ~~[WIP] algorithm='auto' should always work for nearest neighbors (continuation)~~ [MRG] algorithm='auto' should always work for nearest neighbors (continuation) Jun 17, 2017

jnothman approved these changes Jun 17, 2017

View reviewed changes

Customize test for precomputed metric

3ba8c3b

jnothman changed the title ~~[MRG] algorithm='auto' should always work for nearest neighbors (continuation)~~ [MRG+1] algorithm='auto' should always work for nearest neighbors (continuation) Jun 17, 2017

jnothman reviewed Jun 17, 2017

View reviewed changes

Change test function name

aa80bbd

jnothman added this to the 0.19 milestone Jun 18, 2017

amueller reviewed Jun 19, 2017

View reviewed changes

rename _metrics to require_params, add set assert

bb6072a

Checking that the test is non-empty, and more didactic variable name

amueller changed the title ~~[MRG+1] algorithm='auto' should always work for nearest neighbors (continuation)~~ [MRG+2] algorithm='auto' should always work for nearest neighbors (continuation) Jun 19, 2017

Remove blank line

48ea9fa

amueller merged commit 60deaea into scikit-learn:master Jun 19, 2017

This was referenced Jun 19, 2017

[MRG] Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default #5596

Closed

[WIP] algorithm='auto' should always work for nearest neighbors #7669

Closed

herilalaina deleted the iss4931 branch June 19, 2017 19:10

jnothman mentioned this pull request Jun 22, 2017

Fitting a NearestNeighbors model fails with sparse input and a callable as metric #9199

Closed

		assert_array_almost_equal(dist1, dist2)


		def test_algo_auto_metrics():

		assert_array_almost_equal(dist1, dist2)


		def test_unsupported_metric_for_auto():

Uh oh!

Conversation

herilalaina commented Jun 17, 2017

Reference Issue

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

herilalaina commented Jun 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Jun 19, 2017

Uh oh!

herilalaina commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants