8000 [MRG] Svm decision function shape by amueller · Pull Request #4714 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Svm decision function shape #4714

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2015
Merged

[MRG] Svm decision function shape #4714

merged 1 commit into from
Jun 8, 2015

Conversation

amueller
Copy link
Member

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.

@amueller amueller added this to the 0.17 milestone May 12, 2015
@amueller
Copy link
Member Author

In light of #4309, should I do a deprecation of the move of the warning?

@amueller amueller force-pushed the svm_decision_funcition_shape branch 3 times, most recently from e75a6ff to b2b33d4 Compare May 18, 2015 21:08
ogrisel
ogrisel reviewed May 21, 2015
View reviewed changes
sklearn/svm/tests/test_svm.py
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))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need one more similar test on a binary classification problem with compact_decision_function=True.

@ogrisel
Copy link
Member
ogrisel commented May 21, 2015

Other than that LGTM. The new option name is a bit verbose but I don't have a better suggestion

@amueller
Copy link
Member Author

ovo_decision_function? Would maybe be more explicit..

larsmans
larsmans reviewed May 23, 2015
View reviewed changes
sklearn/svm/classes.py
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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is not actually False, it's None. That's going to be confusing for new users; please document the entire tribool.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have we ever documented a default that is None because of a deprecation? Well, I can...

8000
@ogrisel
Copy link
Member
ogrisel commented May 26, 2015

+1 for ovo_decision_function.

@amueller amueller force-pushed the svm_decision_funcition_shape branch 4 times, most recently from 92f0fcf to 5d1edb6 Compare May 27, 2015 19:15
@amueller
Copy link
Member Author

should be good now.

@GaelVaroquaux
Copy link
Member

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'"?

@GaelVaroquaux
Copy link
Member

No other comments than the renaming.

@amueller amueller force-pushed the svm_decision_funcition_shape branch 2 times, most recently from 70fad14 to 35b6190 Compare June 4, 2015 20:22
@amueller
Copy link
Member Author
amueller commented Jun 4, 2015

did the rename.

@amueller amueller force-pushed the svm_decision_funcition_shape branch from 35b6190 to 0eb4cd8 Compare June 4, 2015 21:41
larsmans
larsmans reviewed Jun 5, 2015
View reviewed changes
sklearn/multiclass.py


def _ovr_decision_function(predictions, confidences, n_classes):
"""Compute a continuous, tie-breaking ovo decision function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ovo or ovr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ovr

@amueller amueller force-pushed the svm_decision_funcition_shape branch from 0eb4cd8 to 6d4bcda Compare June 5, 2015 17:14
larsmans
larsmans reviewed Jun 6, 2015
View reviewed changes
sklearn/svm/tests/test_svm.py
clf = ChildSVC()
clf.fit(iris.data, iris.target)
clf.predict(iris.data[-1])
clf.decision_function(iris.data[-1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this gone?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@larsmans larsmans merged commit 6d4bcda into scikit-learn:master Jun 8, 2015
@larsmans
Copy link
Member
larsmans commented Jun 8, 2015

Merged from the command line with manual conflict resolution.

@jnothman
Copy link
Member
jnothman commented Jun 8, 2015

@larsmans, I suspect your conflict resolution created a doctest error. See failing travis: https://travis-ci.org/scikit-learn/scikit-learn/builds/65867028

@larsmans
Copy link
Member
larsmans commented Jun 8, 2015

Fixed in 1274e48. (In my defense, I did run the doctests. I just forgot to commit that change :p )

@amueller amueller mentioned this pull request Jun 8, 2015
Closed
@amueller
Copy link
Member Author
amueller commented Jun 8, 2015

thanks for review + merge :)

@raghavrv raghavrv mentioned this pull request Jun 8, 2015
Merged
24 tasks
@amueller amueller deleted the svm_decision_funcition_shape branch May 19, 2017 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

SVC decision function shape
6 participants
@amueller @ogrisel @GaelVaroquaux @larsmans @jnothman @vene
0