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+1] FIX Negative or null sample_weights in SVM #14286

Merged
merged 40 commits into from Sep 24, 2019

Conversation

@alexshacked
Copy link
Contributor

alexshacked commented Jul 7, 2019

closes #9674
closes #9494

Issue #9494 surfaced when training svm.SVC classifier with negative sample weights. There is a PR that attempted to fix this issue - PR #9674. SVC algorithm is part of five (5) svm algorithms that are implemented in svm.cpp: SVC, NuSVC, SVR, NuSVR and OneClassSVM. Function PREFIX(train) in svm.cpp implements all 5 algorithms. This function handles the samples with negative/zero weights by removing them from the training set.This behavior can result in an invalid model after fit() returns if too many samples are removed. The problem occurs in algorithms: SVC, SVR, NuSVR and OneClassSVM. NuSVC is the only class that currently detects this and fails fit() with an exception.This PR resolves this problem by detecting this situation in all classes and behaving like NuSVC- throwing an Exception during fit(). Please see a more detailed analysis at the bottom of issue #9494

Alex Shacked added 2 commits Jul 6, 2019
…d in weight is not found (#9494)
Alex Shacked added 2 commits Jul 7, 2019
Alex Shacked
Alex Shacked
@rth

This comment has been minimized.

Copy link
Member

rth commented Jul 8, 2019

Thanks for tackling this, @alexshacked ! Wouldn't enforcing that all sample_weights are positive on the Python side (and raising an error if they are not) be a simpler solution?

@alexshacked

This comment has been minimized.

Copy link
Contributor Author

alexshacked commented Jul 8, 2019

Thanks for tackling this, @alexshacked ! Wouldn't enforcing that all sample_weights are positive on the Python side (and raising an error if they are not) be a simpler solution?

Yes, @rth. enforcing that all sample_weights are positive on the python side, in BaseLibSVM.fit() for example would be simpler. This was my original intention.
Until I saw that in the algorithm implementation in svm.cpp:2342, the negative weights are actually handled.
PREFIX(model) *PREFIX(train)(const PREFIX(problem) *prob, const svm_parameter *param, int *status) { PREFIX(problem) newprob; remove_zero_weight(&newprob, prob);
This is the current implementation in the code base. There was a concious decision by the SVM implementor to handle input with negative/zero weights, by removing this samples from the input, and training the model on the remaining samples. The SVM implementor was not shy about rejecting invalid input and in function check_parameter() it does just that for many scenarios. But in the case of negative weights the decision was not to reject the input but to handle it by removing samples with negative weights.
The only thing is that while doing so the SVM implementor ommited some border cases that arise, like the one discovered by bug #9494.
My thought was that a less intrusive fix would be to teach the implementation to handle the border cases without changing current behavior of the SVM algorithm which handles input with negative weights.
Hence, the present solution.
If on the other hand, the decision for SVM algorithms would be not to handle input where part of the samples are zero/negative anymore, I would be happy to implement. On the python side naturally:-)

@rth rth changed the title [MRG] #9494 FIX Fail to train SVM ... [MRG] #9494 FIX Negative or null sample_weights in SVM Jul 8, 2019
@rth rth changed the title [MRG] #9494 FIX Negative or null sample_weights in SVM [MRG] FIX Negative or null sample_weights in SVM Jul 8, 2019
@glemaitre glemaitre self-requested a review Jul 8, 2019
@alexshacked

This comment has been minimized.

Copy link
Contributor Author

alexshacked commented Jul 8, 2019

Hi @glemaitre. I don't know what self-request a review means. But, I"ll be glad if you will review my PR.
I see that you also have worked with sample weights lately
If you need me to do something to enable you to review please tell me.

Copy link
Contributor

glemaitre left a comment

I am proposing some changes in the tests:

  • use pytest
  • improve some previous tests which were using sample_weight

The tests will fail because the ValueError raised does not match the correct error message (which is hidden when using assert_raises).

Even if I agree with the changes in the cpp file, I think that we should implement what was proposed by @rth. Validate and failing early will be better. So I would add the checking in the cpp and change slightly the tests that I propose.

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Jul 8, 2019

self-request a review means

@alexshacked it is just for me to track the PR which I am going to review :)

Copy link
Contributor

glemaitre left a comment

Style change in the CPP file

sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Show resolved Hide resolved
alexshacked and others added 14 commits Jul 9, 2019
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Jul 17, 2019

It looks good for me. We need a second approval.

@glemaitre glemaitre changed the title [MRG] FIX Negative or null sample_weights in SVM [MRG+1] FIX Negative or null sample_weights in SVM Jul 17, 2019
@alexshacked

This comment has been minimized.

Copy link
Contributor Author

alexshacked commented Jul 18, 2019

It looks good for me. We need a second approval.

Ok. @rth, @glemaitre thanx again for your help with this issue. I guess there remains not much for me to do for this PR. I will look for a new issue on scikit-learn to work on.

sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
sklearn/svm/src/libsvm/svm.cpp Outdated Show resolved Hide resolved
@amueller amueller added the Bug label Sep 19, 2019
@amueller amueller added this to the 0.22 milestone Sep 19, 2019
@glemaitre glemaitre added this to TO REVIEW in Guillaume's pet Sep 20, 2019
Copy link
Contributor

glemaitre left a comment

LGTM

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
@glemaitre glemaitre moved this from TO REVIEW to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet Sep 23, 2019
@glemaitre glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to GETTING STALLED in Guillaume's pet Sep 23, 2019
@glemaitre glemaitre moved this from GETTING STALLED to TO BE MERGED in Guillaume's pet Sep 23, 2019
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Sep 24, 2019

@jnothman do you want to have a new pass on it.

clf = svm.SVC()
clf.fit(X, Y)
assert_array_equal(clf.predict([X[2]]), [1.])
@pytest.mark.parametrize("estimator", [svm.SVC(C=1e-2), svm.NuSVC()])

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

This test doesn't fail in master for me. Should it?

This comment has been minimized.

Copy link
@alexshacked

alexshacked Sep 24, 2019

Author Contributor

This test doesn't check zero or negative weights. I added it to the unit tests for this feature as asked by @glemaitre who was working on another feature related to weights in SVM. Probably that feature is already merged.

This comment has been minimized.

Copy link
@glemaitre

glemaitre Sep 24, 2019

Contributor

If you check the previous test_sample_weights, it was not testing anything from the sample_weight.
The added tests do not fail because it was no bug because at least they are added to be sure that the results are rights. It could be done in another PR but I wanted to avoid splitting it for simplicity for a first contrib :)

"estimator",
[svm.SVR(C=1e-2), svm.NuSVR(C=1e-2)]
)
def test_svm_regressor_sided_sample_weight(estimator):

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

This test doesn't fail in master for me. Should it?

This comment has been minimized.

Copy link
@alexshacked

alexshacked Sep 24, 2019

Author Contributor

This test doesn't check zero or negative weights. I added it to the unit tests for this feature as asked by @glemaitre who was working on another feature related weights in SVM. Probably that feature is already merged.

[[1, -0.5, 1, 1, 1, 1], [1, 1, 1, 0, 1, 1]],
ids=['partial-mask-label-1', 'partial-mask-label-2']
)
def test_negative_weight_equal_coeffs(Estimator, sample_weight):

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

This test doesn't fail in master for me. Should it?

This comment has been minimized.

Copy link
@alexshacked

alexshacked Sep 24, 2019

Author Contributor

This test shouldn't fail. It does not verify the main goal of the feature - throw Exception during fit() if samples of one label are totaly removed because they all had zero or negative weghts.
Insted this test validates the "healthy behaviour" of current model. That is, If only a few samples are removed and the ratio between the samples with diferent labels is not severely disrupted, then the classifier's model doesen't change much. The model coefficients that were originally equal, still remain close to each other.
Since this test doesen't actually verify the feature I intend to add, I can remove the test if you think we are better without it

This comment has been minimized.

Copy link
@alexshacked

alexshacked Sep 24, 2019

Author Contributor

I added the test, thinking it can detect future changes to the classifier model which will change this present behaviour.

@glemaitre glemaitre moved this from TO BE MERGED to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet Sep 24, 2019
Copy link
Member

jnothman left a comment

Thanks @alexshacked!

@jnothman jnothman merged commit d6d1d63 into scikit-learn:master Sep 24, 2019
19 checks passed
19 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
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.74%)
Details
codecov/project 96.75% (+0.01%) compared to 746efb5
Details
scikit-learn.scikit-learn Build #20190923.24 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
@glemaitre glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to MERGED in Guillaume's pet Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.