[MRG] Fix ZeroDivisionError when using sparse data in SVM in case where support_vectors_ is empty #14894
Conversation
|
@danna-naser thanks for reporting it and the fix. But it may be the case that there's another underlying issue we need to solve. @agramfort could you please have a look at this one? The issue is that sometimes you may get a solution from the SVR with 0 support vectors, and the output is just the intercept. The question is, do we want to raise a warning or even an error? Also, this example is very curious in the sense that the intercept is not the mean of all the data points. import numpy as np
from scipy import sparse
from sklearn import svm
x_train = np.array([[0, 1, 0, 0],
[0, 0, 0, 1],
[0, 0, 1, 0],
[0, 0, 0, 1]])
y_train = np.array([0.04, 0.04, 0.10, 0.16])
model = svm.SVR(kernel='linear')
model.fit(x_train, y_train) |
|
@adrinjalali any update on this? If an underlying issue was found, please let me know and I'll close :) |
|
@NicolasHug what do you think of this issue? |
|
I'm not sure what the intercept should be but since the dense version also has no SV, the fix looks correct to me? |
There was a problem hiding this comment.
I'm convinced this fixes the issue at hand, still not sure why this is happening (no SV I mean). But I'm happy to have this in. Thanks @danna-naser
There was a problem hiding this comment.
Thanks for the PR @danna-naser ! Some minor nits but LGTM anyway
sklearn/svm/base.py
Outdated
| self.dual_coef_ = sp.csr_matrix( | ||
| (dual_coef_data, dual_coef_indices, dual_coef_indptr), | ||
| (n_class, n_SV)) | ||
| if dual_coef_indices.size == 0: |
There was a problem hiding this comment.
I would replace this check with if n_SV == 0 and only declare dual_coef_indices where it is actually used, i.e. in the else clause.
There was a problem hiding this comment.
replaced with not n_SV check. If you'd prefer n_SV == 0 , just let me know
sklearn/svm/tests/test_svm.py
Outdated
| y_train = np.array([0.04, 0.04, 0.10, 0.16]) | ||
| model = svm.SVR(kernel='linear') | ||
| model.fit(X_train, y_train) | ||
| assert model.support_vectors_.data.size == 0 |
There was a problem hiding this comment.
model.support_vectors_.size is enough (same below)
There was a problem hiding this comment.
I made the change. Please let me know if anything else can be improved!
| @@ -560,6 +560,19 @@ def test_sparse_precomputed(): | |||
| assert "Sparse precomputed" in str(e) | |||
|
|
|||
|
|
|||
| def test_sparse_fit_support_vectors_empty(): | |||
| # Regression test for #14893 | |||
There was a problem hiding this comment.
Wow Github is linking to the issue that's pretty cool
3906328 to
0e58862
Compare
|
Thanks @danna-naser ! |
Reference Issues/PRs
Fixes #14893 #14893
What does this implement/fix? Explain your changes.
When model.support_vectors_ is an empty sparse matrix, to calculate model.dual_coef_, we use
which results in ZeroDivisionError.
This change skips this calculation in this case and makes the model.dual_coef_ consistent in dense vs sparse data
Any other comments?