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.

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

[MRG+1] Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric #9579

Merged
merged 5 commits into from Dec 6, 2017

Conversation

Projects
None yet
3 participants
@tttthomasssss
Contributor

tttthomasssss commented Aug 17, 2017

Reference Issue

Fixes #9199

What does this implement/fix? Explain your changes.

The fix simply extends the current sparse input incompatibility check with a callable check.

Any other comments?

A test is included in test_neighbors.py

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 18, 2017

Member

Unfortunately the earliest supported version of scipy does not have count_nonzero. Is np.diff(X.indptr) sufficient?

Also, you should use PEP8 to pass our tests.

Member

jnothman commented Aug 18, 2017

Unfortunately the earliest supported version of scipy does not have count_nonzero. Is np.diff(X.indptr) sufficient?

Also, you should use PEP8 to pass our tests.

@tttthomasssss

This comment has been minimized.

Show comment
Hide comment
@tttthomasssss

tttthomasssss Aug 23, 2017

Contributor

I've just modified my PR - apologies for the pep8 mess...

Contributor

tttthomasssss commented Aug 23, 2017

I've just modified my PR - apologies for the pep8 mess...

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 23, 2017

Member

Tests are failing. Can you please make the title of the PR more descriptive as well?

Member

jnothman commented Aug 23, 2017

Tests are failing. Can you please make the title of the PR more descriptive as well?

@tttthomasssss tttthomasssss changed the title from Fixes #9199 to Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric Aug 24, 2017

@tttthomasssss

This comment has been minimized.

Show comment
Hide comment
@tttthomasssss

tttthomasssss Aug 24, 2017

Contributor

@jnothman - do you know which ones are affected by my change? I've removed my fix and the same tests (+ the one I added) are failing in comparison to with my fix (- the one I added).

Contributor

tttthomasssss commented Aug 24, 2017

@jnothman - do you know which ones are affected by my change? I've removed my fix and the same tests (+ the one I added) are failing in comparison to with my fix (- the one I added).

@jnothman jnothman changed the title from Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric to [MRG+1] Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric Sep 4, 2017

@jnothman jnothman changed the title from [MRG+1] Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric to Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric Sep 4, 2017

@tttthomasssss

This comment has been minimized.

Show comment
Hide comment
@tttthomasssss

tttthomasssss Sep 21, 2017

Contributor

OK, finally managed to get back to this, the metric now returns the dot product of the inputs

Contributor

tttthomasssss commented Sep 21, 2017

OK, finally managed to get back to this, the metric now returns the dot product of the inputs

@jnothman jnothman changed the title from Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric to [MRG+1] Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric Sep 26, 2017

@jnothman

Thanks. LGTM. Properly this time :)

@TomDLT

This comment has been minimized.

Show comment
Hide comment
@TomDLT

TomDLT Dec 6, 2017

Member

LGTM, thanks for the fix @tttthomasssss

Member

TomDLT commented Dec 6, 2017

LGTM, thanks for the fix @tttthomasssss

@TomDLT TomDLT merged commit ece341c into scikit-learn:master Dec 6, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.16%)
Details
codecov/project 96.17% (+0.01%) compared to aae8700
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 6, 2017

Member

There's no what's new entry. My bad :(

@tttthomasssss, would you be able to open a new PR which adds an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

Member

jnothman commented Dec 6, 2017

There's no what's new entry. My bad :(

@tttthomasssss, would you be able to open a new PR which adds an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

tttthomasssss added a commit to tttthomasssss/scikit-learn that referenced this pull request Dec 14, 2017

jnothman added a commit that referenced this pull request Dec 14, 2017

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment