[MRG] SVM: Ensure nonnegative sample weights (#9494) #9674
Conversation
|
Uhm no chances on this one :) |
|
@glemaitre, do you mean that the original issue should be closed without any code change, or that there is something fundamentally wrong with my try to do test driven programming? |
|
Oh sorry, actually the CI did not run and thus I believed that you test did not failed. |
|
I see. Bad nightly idea to deactivate CI on that. I will switch it back on, s.t. you don't need to test locally. |
e9b3329 to
f6d6fe3
Compare
|
FWIW, A travis build additionally shows the stderr message |
|
No, I guess the warning must be from the corresponding place in svm.cpp, since the test calls Actually, this warning might just be a side effect (or even totally unrelated), because the code there is about label weights |
|
(See my comment in the issue.) |
|
Add a fix? |
f6d6fe3 to
744862c
Compare
744862c to
66060f7
Compare
|
Ready for review. |
sklearn/svm/base.py
Outdated
| @@ -170,6 +170,9 @@ def fit(self, X, y, sample_weight=None): | |||
| "boolean masks (use `indices=True` in CV)." | |||
| % (sample_weight.shape, X.shape)) | |||
|
|
|||
| if sample_weight.shape[0] > 0 and min(sample_weight) < 0: | |||
sklearn/svm/tests/test_svm.py
Outdated
|
|
||
|
|
||
| def test_negative_weights(): | ||
| W = np.array([1, 1, 1, -1, -1, -1]) |
|
I feel this should be with a check_sample_weights function tested on all estimators with a common test that can be disable for speed with the new context manager. |
As needed for LIBSVM and LIBLINEAR.
66060f7 to
2af259d
Compare
|
@agramfort: I guess Should the disabling look as in #7548? E.g. with sklearn.config_context(assume_positive_sample_weights=True):
clf.fit(X, Y, sample_weight=sample_weights) |
|
we don't usually put much in base.py. our approach is rather to create a
common test, then modify whichever estimators are applicable. A shared
utility function would go in sklearn/utils/validation.py
…On 11 Sep 2017 7:07 am, "Robert Pollak" ***@***.***> wrote:
@agramfort <https://github.com/agramfort>:
I have applied the small changes. Would (sample_weight < 0).any() be even
better?
I guess sklearn/base.pyis the place to call check_sample_weights for all
estimators? And utils/validation.py the place to define it? But does no
estimator support negative weights?
Should the disabling look like in #7548
<#7548>? E.g.
with sklearn.config_context(assume_positive_sample_weights=True):
clf.fit(X, Y, sample_weight=sample_weights)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9674 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66V812BByNQ2I8O9G5OouDCLcyswks5shE9zgaJpZM4PKvfZ>
.
|
|
agreed with @jnothman
I think adaboost can have negative weights. I remember a conversation or
it's my troubled mind ...
|
|
Besides I even don't know about the other svm estimators. I guess that at least |
|
Yes, so we could have a common test which checks for an appropriate error
message or else no error.
…On 12 September 2017 at 06:04, Robert Pollak ***@***.***> wrote:
Besides svm.SVC, are there other estimators that need nonnegative
weights? I found no such case in the sklearn docs. (Should I document it
for svm.SVC?)
I even don't know about the other svm estimators
<http://scikit-learn.org/stable/modules/classes.html#estimators>. I guess
that at least svm.LinearSVC might also need nonnegative weights, since
the README.weight of LIBLINEAR also says "Please make sure all weights
are non-negative". So the common test can be reused at least here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9674 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz68sDP1c2Qc3OzWrCSHGL1O3P9s97ks5shZJJgaJpZM4PKvfZ>
.
|
|
I think most people assume weights are non-negative, but as @agramfort said, they don't need to be for adaboost (I'm not sure about gradient boosting and other tree-based algorithms). Can you run a test over |
|
Actually the SVC algorithm handles negative weights. It happens in the implementation in function PREFIX(train) in svm.cpp:2338. The first thing this function does is calling remove_zero_weight(), located also in svm.cpp. remove_zero_weight() will remove from the input all the samples which have a negative or zero weight. So the training algorithm will process only the samples with positive weights. The problem in the specific example that uncovered bug #9494 is that here all samples that had the "0" label also received a negative weight. So after removing the samples with negative weights the training algorithm in svm.cpp was left with only the samples having label "1". This is of course an invalid state and this is why coef_ was not created. For SVM training the input must contain samples with label "0" and also samples with label "1". Both classes must be present in the training data. A more encapsulated fix could be done in the implementation - PREFIX(train) in svm.cpp I see that this issue was not handled for some time, and would be happy to take it on and handle the pull-request. |
|
@amueller, @agramfort I saw that issue #9494 (which is handled by this PR), was tagged "help wanted". |
|
If we can make the change in python land rather than CPP land, that would
be preferable, but your solution would also be welcome as a pull request,
thanks.
|
|
Opened PR #14282 |
As a first step for dealing with #9494, this reproduces the issue in a unit test.
This should fail with the same IndexError as in the original report.
It only fails when all of the weights of the second class are negative.