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] ENH add a parameter to SVC and NuSVC to break ties in predict if desired #12557

Merged
merged 35 commits into from May 3, 2019

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Nov 10, 2018

Fixes #8277

This is a proposal to fix the issue in SVC and NuSVC where the predict does not break ties but decision_function does, when decision_function_shape='ovr' and n_classes > 2

The proposal is to add a parameter to BaseSVC, and return argmax of decision_function instead of calling libsvm's predict, which breaks ties according to the confidence values.

The default value of the parameter is False to keep it backward compatible, and if needed, in the future, switch the default value to True in an appropriate deprecation cycle.

@agramfort
Copy link
Member

@agramfort agramfort commented Nov 11, 2018

@adrinjalali travis is not happy.

Copy link
Member

@jnothman jnothman left a comment

Thanks @adrinjalali

sklearn/svm/classes.py Outdated Show resolved Hide resolved
sklearn/svm/classes.py Outdated Show resolved Hide resolved
@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Nov 14, 2018

I'm not sure what whats_new tags to use here though.

@jnothman
Copy link
Member

@jnothman jnothman commented Nov 14, 2018

I'm not sure what whats_new tags to use here though.

It's an enhancement, IMO.

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
sklearn/svm/classes.py Outdated Show resolved Hide resolved
Copy link
Member

@jnothman jnothman left a comment

Nitpicks

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
@adrinjalali adrinjalali changed the title add a parameter to SVC and NuSVC to break ties in predict if desired [MRG+1] add a parameter to SVC and NuSVC to break ties in predict if desired Nov 28, 2018
@adrinjalali adrinjalali changed the title [MRG+1] add a parameter to SVC and NuSVC to break ties in predict if desired [MRG+1] ENH add a parameter to SVC and NuSVC to break ties in predict if desired Nov 29, 2018
@adrinjalali adrinjalali added this to the 0.21 milestone Dec 19, 2018
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 13, 2019

Tests failing?

@adrinjalali adrinjalali added this to Needs review in Sprint Paris 2019 Feb 28, 2019
Copy link
Member

@rth rth left a comment

Looks good apart for one comment below.

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

LGTM, aside from my two comments. The naming is a nitpick which can be ignored, but I feel that the random_state is important.

sklearn/svm/tests/test_svm.py Outdated Show resolved Hide resolved
@jnothman
Copy link
Member

@jnothman jnothman commented Apr 15, 2019

I can't see the difference between those images.

@azrdev
Copy link

@azrdev azrdev commented Apr 15, 2019

I can't see the difference between those images.

@jnothman It's the area "inside" the intersection of the three borders, at around (1.5, 8) which has a concave extension of the top-left green class, iff tie breaking is used

@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Apr 15, 2019

@jnothman I increased the plot size a bit, I hope that helps.

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 20, 2019

I find it hard to see the difference between the two plots (took me more than a minute to see that the regions weren't the same at the center).

The description should explain what differences to look for IMO.

@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Apr 22, 2019

@thomasjpfan I'm not sure why the two azure pipelines are failing, the failing test is not touched by this PR (and I can't figure what the error is anyway).

EDIT: is/was fixed on master

@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Apr 23, 2019

@NicolasHug does this look good now?

Copy link
Member

@NicolasHug NicolasHug left a comment

Looks good, a few comments still.

Not sure if we want to raise an error if break_ties is True and decision_function_shape is not 'ovr'. I'd say yes but apparently the default is subject to change?

doc/modules/svm.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
examples/svm/plot_svm_tie_breaking.py Outdated Show resolved Hide resolved
sklearn/svm/classes.py Outdated Show resolved Hide resolved
sklearn/svm/classes.py Outdated Show resolved Hide resolved
doc/modules/svm.rst Outdated Show resolved Hide resolved
@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Apr 26, 2019

Not sure if we want to raise an error if break_ties is True and decision_function_shape is not 'ovr'. I'd say yes but apparently the default is subject to change?

I'm not sure. I'd leave that to another discussion maybe.

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 26, 2019

I'm not sure. I'd leave that to another discussion maybe.

If we merge this before 0.21, it will be too late. I think we should raise an error, we've been very conservative so far on what users can do.

@adrinjalali
Copy link
Member Author

@adrinjalali adrinjalali commented Apr 29, 2019

@NicolasHug done :)

Copy link
Member

@NicolasHug NicolasHug left a comment

Last comment, LGTM otherwise!

doc/modules/svm.rst Outdated Show resolved Hide resolved
sklearn/svm/base.py Show resolved Hide resolved
@NicolasHug NicolasHug merged commit 384c8ad into scikit-learn:master May 3, 2019
16 checks passed
Sprint Paris 2019 automation moved this from Needs review to Done May 3, 2019
@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented May 3, 2019

Thanks Adrin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

8 participants