FIX LabelPropagation handling of sparce matrices #17085 #17384
Conversation
There was a problem hiding this comment.
Thanks for the PR @goerch . This also needs a regression test specific to the issue it fixes.
| @@ -190,7 +190,7 @@ class labels. | |||
| """ | |||
| check_is_fitted(self) | |||
|
|
|||
| X_2d = check_array(X, accept_sparse=['csc', 'csr', 'coo', 'dok', | |||
| X_2d = check_array(X, accept_sparse=['csc', 'csr', 'coo', | |||
There was a problem hiding this comment.
should this use _validate_data? ping @NicolasHug
There was a problem hiding this comment.
I'm also really not sure why this fixes the issue
There was a problem hiding this comment.
should this use _validate_data
We only use it in fit for now, so check_array is fine here
There was a problem hiding this comment.
Thanks for the feedback @adrinjalali: PR #17085 already contains tests extended to the sparse case. Could you please advise a newbie regarding regression tests?
| clf = label_propagation.LabelSpreading(max_iter=10000, alpha=alpha) | ||
| clf.fit(X, y) | ||
| assert_array_almost_equal(expected, clf.label_distributions_, 4) | ||
| for sparse_or_dense in SPARSE_OR_DENSE: |
There was a problem hiding this comment.
could you please parameterize the test for this one and alpha? Also for the other tests
There was a problem hiding this comment.
@adrinjalali: I didn't write the original tests, so I'm probably not the right one to ask for improving these with regard to parametrization.
|
I am taking over this issue in #19664 |
|
I am closing this pull request as superseded by #19664. |
Reference Issues/PRs
Fixes #17085
What does this implement/fix? Explain your changes.
Label propagation and spreading allow to classify using sparse data according to documentation. Tests only covered the dense case. Newly added coverage for sparse matrices allows to reproduce the problem in #17085. The proposed fix in #17085 works for the extended tests.
Any other comments?
test_label_propagation_closed_formfails for sparse matrices