[MRG+1] Fixes #9199 - Fitting a NearestNeighbors model fails with sparse input and a callable as metric #9579
Conversation
…ghbors model fails with sparse input and a callable as metric
|
Unfortunately the earliest supported version of scipy does not have count_nonzero. Is Also, you should use PEP8 to pass our tests. |
|
I've just modified my PR - apologies for the |
|
Tests are failing. Can you please make the title of the PR more descriptive as well? |
|
@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). |
| @@ -1255,6 +1255,35 @@ def test_dtype_convert(): | |||
| assert_array_equal(result, y) | |||
|
|
|||
|
|
|||
| def test_sparse_metric_callable(): | |||
| def sparse_metric(x, y): # Metric accepting sparse matrix input (only) | |||
|
|
|||
There was a problem hiding this comment.
remove blank line, or put comment here.
| def test_sparse_metric_callable(): | ||
| def sparse_metric(x, y): # Metric accepting sparse matrix input (only) | ||
|
|
||
| return min(x.nnz, y.nnz) / max(x.nnz, y.nnz) |
There was a problem hiding this comment.
Sorry, I have to take back the LGTM. :\ This is apparently producing different results on old scipy, and tests are failing. Perhaps just assert isspmatrix(x) and return some more ordinary function of the inputs.
…e inputs, provided they are sparse
|
OK, finally managed to get back to this, the metric now returns the dot product of the inputs |
|
LGTM, thanks for the fix @tttthomasssss |
|
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 |
…ils with sparse input and a callable as metric (scikit-learn#9579)
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