Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

preddy5
Copy link

@preddy5 preddy5 commented Oct 26, 2015

@MechCoder
Copy link
Member

Can you add tests?

@preddy5
Copy link
Author

preddy5 commented Oct 28, 2015

Ya, working on it

@giorgiop
Copy link
Contributor

Shall we update the docstring as well?

@ogrisel ogrisel changed the title issue 4931 Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default Oct 28, 2015
@ogrisel
Copy link
Member

ogrisel commented Oct 28, 2015

Yes the docstring needs to be updated. I changed the title of this PR to make it more explicit. @pradyu1993 please add a [MRG] prefix to the title of this PR once you are done addressing the comments.

@ogrisel ogrisel changed the title Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default [WIP] Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default Oct 28, 2015
@preddy5 preddy5 changed the title [WIP] Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default [MRG] Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default Oct 29, 2015
@preddy5
Copy link
Author

preddy5 commented Nov 1, 2015

@MechCoder can you review the PR

@jnothman
Copy link
Member

jnothman commented Nov 1, 2015

Please also test with sparse matrix input

@preddy5 preddy5 force-pushed the iss4931 branch 2 times, most recently from 63ad4e4 to b969bf6 Compare November 2, 2015 05:31
@preddy5
Copy link
Author

preddy5 commented Nov 3, 2015

@jnothman Done

@jnothman
Copy link
Member

jnothman commented Nov 3, 2015

I intended that you would check that metrics which are supported for dense data but not for sparse data will still work in auto. Or is that covered in another test?

@preddy5
Copy link
Author

preddy5 commented Dec 7, 2015

@MechCoder can you review the PR

MechCoder
MechCoder reviewed Dec 20, 2015
View reviewed changes
sklearn/neighbors/tests/test_neighbors.py
Xcsr = csr_matrix(X)
Valid_Metrics = dict(dense = ['precomputed', 'sqeuclidean',
'yule', 'cosine', 'correlation'],
sparse=PAIRWISE_DISTANCE_FUNCTIONS.keys())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do away with this and just check for VALID_METRICS['brute'] and VALID_METRICS_SPARSE['brute']. I think the other options are checked already.

@MechCoder
Copy link
Member

Thanks ! Just some minor comments. And then +1

But you might have to wait till after the holiday season for the second ^.^

@jnothman
Copy link
Member

@pradyu1993 are you still working on this?

@jnothman
Copy link
Member

jnothman commented Oct 8, 2016

This is up for grabs by a contributor as far as I'm concerned.

@jnothman jnothman added the Easy Well-defined and straightforward way to resolve label Oct 8, 2016
@jnothman jnothman mentioned this pull request Oct 8, 2016
Closed
@amueller amueller added this to the 0.19 milestone Oct 11, 2016
@maniteja123
Copy link
Contributor

Hi @jnothman @amueller I can cherry pick this commit in case no one is working and do it as part of #7590 . Would it be fine ?

@jnothman
Copy link
Member

I'd rather this be patched separate to #7590...

On 11 October 2016 at 17:47, Maniteja Nandana notifications@github.com
wrote:

Hi @jnothman https://github.com/jnothman @amueller
https://github.com/amueller I can cherry pick this commit in case no
one is working and do it as part of #7590
#7590 . Would it be
fine ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5596 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz61XAlHHbexSm5CUvNcwMyYI2wRjbks5qyzDlgaJpZM4GVpFN
.

@amueller
Copy link
Member

👍 for small PRs, also they seem pretty unrelated (or I'm missing something, which is entirely plausible after reading like 5000 notifications this week)

@maniteja123 maniteja123 mentioned this pull request Oct 14, 2016
Closed
@herilalaina herilalaina mentioned this pull request Jun 17, 2017
Merged
@amueller
Copy link
Member

fixed by #9145.

@amueller amueller closed this Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Easy Well-defined and straightforward way to resolve
Projects
None yet
Milestone
0.19
Development

Successfully merging this pull request may close these issues.
7 participants
@preddy5 @MechCoder @giorgiop @ogrisel @jnothman @maniteja123 @amueller