Skip to content

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

Closed
preddy5 wants to merge 1 commit intoscikit-learn:masterfrom
preddy5:iss4931
Closed

[MRG] Fix NearestNeighbors algorithm='auto' to work with all supported metrics by default#5596
preddy5 wants to merge 1 commit intoscikit-learn:masterfrom
preddy5:iss4931

Conversation

@preddy5
Copy link
Copy Markdown

@preddy5 preddy5 commented Oct 26, 2015

@MechCoder
Copy link
Copy Markdown
Member

MechCoder commented Oct 27, 2015

Can you add tests?

@preddy5
Copy link
Copy Markdown
Author

preddy5 commented Oct 28, 2015

Ya, working on it

@giorgiop
Copy link
Copy Markdown
Contributor

giorgiop commented Oct 28, 2015

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
Copy Markdown
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
Copy Markdown
Author

preddy5 commented Nov 1, 2015

@MechCoder can you review the PR

@jnothman
Copy link
Copy Markdown
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
Copy Markdown
Author

preddy5 commented Nov 3, 2015

@jnothman Done

@jnothman
Copy link
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
Member

@MechCoder MechCoder Dec 20, 2015

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
Copy Markdown
Member

MechCoder commented Dec 20, 2015

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
Copy Markdown
Member

jnothman commented Apr 26, 2016

@pradyu1993 are you still working on this?

@jnothman
Copy link
Copy Markdown
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
Copy Markdown
Contributor

maniteja123 commented Oct 11, 2016

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
Copy Markdown
Member

jnothman commented Oct 12, 2016

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
Copy Markdown
Member

amueller commented Oct 13, 2016

👍 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
Copy Markdown
Member

amueller commented Jun 19, 2017

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