[MRG] Svm decision function shape #4714
Conversation
|
In light of #4309, should I do a deprecation of the move of the warning? |
amueller
force-pushed
the
svm_decision_funcition_shape
branch
3 times, most recently
from
e75a6ff
to
b2b33d4
Compare
| dec = clf.decision_function(X_test) | ||
| assert_array_equal(clf.predict(X_test), np.argmax(dec, axis=1)) | ||
| assert_equal(dec.shape, (len(X_test), 5)) | ||
|
|
There was a problem hiding this comment.
I think we need one more similar test on a binary classification problem with compact_decision_function=True.
|
Other than that LGTM. The new option name is a bit verbose but I don't have a better suggestion |
|
ovo_decision_function? Would maybe be more explicit.. |
| Whether to return a decision function of shape (n_samples, n_classes) | ||
| as all other classifiers, or the original one-vs-one decision function | ||
| of libsvm which has shape (n_samples, n_classes * (n_classes - 1) / 2). | ||
|
|
There was a problem hiding this comment.
The default is not actually False, it's None. That's going to be confusing for new users; please document the entire tribool.
There was a problem hiding this comment.
have we ever documented a default that is None because of a deprecation? Well, I can...
|
+1 for |
amueller
force-pushed
the
svm_decision_funcition_shape
branch
4 times, most recently
from
92f0fcf
to
5d1edb6
Compare
|
should be good now. |
|
I hate to say, but I really don't like the name. I find it hard to relate to what it does. Maybe "decision_shape='ovo'"? |
|
No other comments than the renaming. |
amueller
force-pushed
the
svm_decision_funcition_shape
branch
2 times, most recently
from
70fad14
to
35b6190
Compare
|
did the rename. |
amueller
force-pushed
the
svm_decision_funcition_shape
branch
from
35b6190
to
0eb4cd8
Compare
|
|
||
|
|
||
| def _ovr_decision_function(predictions, confidences, n_classes): | ||
| """Compute a continuous, tie-breaking ovo decision function. |
…cision_function shape
amueller
force-pushed
the
svm_decision_funcition_shape
branch
from
0eb4cd8
to
6d4bcda
Compare
| clf = ChildSVC() | ||
| clf.fit(iris.data, iris.target) | ||
| clf.predict(iris.data[-1]) | ||
| clf.decision_function(iris.data[-1]) |
There was a problem hiding this comment.
Hmm, just curious, what could you do to mess up the SVC class to make this fail? (but so that it wouldn't fail if clf=SVC()?)
There was a problem hiding this comment.
I felt it was a very odd tests. I also have no idea how this could fail. And I don't see why we would test that for SVC but not anything else. Maybe it was not a great idea to do this in this PR.
There was a problem hiding this comment.
Apparently the old SVM class hierarchy didn't admit inheritance because attributes were being deleted; see beda3f4. Since the hierarchy was completely refactored since then, I've no strong objections to removing the test, but I don't mind keeping it either.
|
Merged from the command line with manual conflict resolution. |
|
@larsmans, I suspect your conflict resolution created a doctest error. See failing travis: https://travis-ci.org/scikit-learn/scikit-learn/builds/65867028 |
|
Fixed in 1274e48. (In my defense, I did run the doctests. I just forgot to commit that change :p ) |
|
thanks for review + merge :) |
This adds an option to change the shape of SVC.decision_function to a scikit-learn compatible shape.
This will allow people to use it with CalibratedClassifierCV in a multi-class setting, and also just be much more api-friendly.
I reused the function from the OvO multiclass classifier.
Fixes #4638, #4634.
ping @mblondel.