Skip to content
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] Fix ZeroDivisionError when using sparse data in SVM in case where support_vectors_ is empty #14894

Merged
merged 10 commits into from Oct 7, 2019

Conversation

@danna-naser
Copy link
Contributor

danna-naser commented Sep 5, 2019

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

dual_coef_indptr = np.arange(0, dual_coef_indices.size + 1,
                                         dual_coef_indices.size / n_class)

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?

@danna-naser danna-naser changed the title [WIP] Fix ZeroDivisionError when using sparse data in SVM in case where support_vectors_ is empty [MRG] Fix ZeroDivisionError when using sparse data in SVM in case where support_vectors_ is empty Sep 6, 2019
@adrinjalali

This comment has been minimized.

Copy link
Member

adrinjalali commented Sep 6, 2019

@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)
@danna-naser

This comment has been minimized.

Copy link
Contributor Author

danna-naser commented Sep 30, 2019

@adrinjalali any update on this? If an underlying issue was found, please let me know and I'll close :)

@adrinjalali

This comment has been minimized.

Copy link
Member

adrinjalali commented Oct 1, 2019

@NicolasHug what do you think of this issue?

@NicolasHug

This comment has been minimized.

Copy link
Contributor

NicolasHug commented Oct 1, 2019

I'm not sure what the intercept should be but since the dense version also has no SV, the fix looks correct to me?

Copy link
Member

adrinjalali left a 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

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
Copy link
Member

adrinjalali left a comment

Otherwise LGTM.

sklearn/svm/tests/test_svm.py Show resolved Hide resolved
Copy link
Contributor

NicolasHug left a comment

Thanks for the PR @danna-naser ! Some minor nits but LGTM anyway

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:

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Oct 7, 2019

Contributor

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.

This comment has been minimized.

Copy link
@danna-naser

danna-naser Oct 7, 2019

Author Contributor

replaced with not n_SV check. If you'd prefer n_SV == 0 , just let me know

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

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Oct 7, 2019

Contributor

model.support_vectors_.size is enough (same below)

This comment has been minimized.

Copy link
@danna-naser

danna-naser Oct 7, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Oct 7, 2019

Contributor

Wow Github is linking to the issue that's pretty cool

Copy link
Member

rth left a comment

Please add an entry to the change log at doc/whats_new/v0.22.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.

LGTM, otherwise.

@danna-naser danna-naser force-pushed the danna-naser:sparse_svm_divide_by_zero branch from 3906328 to 0e58862 Oct 7, 2019
@NicolasHug NicolasHug merged commit a89462b into scikit-learn:master Oct 7, 2019
19 checks passed
19 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.01%)
Details
codecov/project 96.85% (+0.83%) compared to fdbaa58
Details
scikit-learn.scikit-learn Build #20191007.30 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
@NicolasHug

This comment has been minimized.

Copy link
Contributor

NicolasHug commented Oct 7, 2019

Thanks @danna-naser !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.