[MRG] Svm decision function shape#4714
Conversation
|
In light of #4309, should I do a deprecation of the move of the warning? |
e75a6ff to
b2b33d4
Compare
sklearn/svm/tests/test_svm.py
Outdated
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.. |
sklearn/svm/classes.py
Outdated
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 |
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. |
70fad14 to
35b6190
Compare
|
did the rename. |
35b6190 to
0eb4cd8
Compare
sklearn/multiclass.py
Outdated
…cision_function shape
0eb4cd8 to
6d4bcda
Compare
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 ) |
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.