[MRG+1] ENH add a parameter to SVC and NuSVC to break ties in predict if desired#12557
Conversation
|
@adrinjalali travis is not happy. |
|
I'm not sure what |
It's an enhancement, IMO. |
645ed71 to
489f734
Compare
|
Tests failing? |
rth
left a comment
There was a problem hiding this comment.
Looks good apart for one comment below.
GaelVaroquaux
left a comment
There was a problem hiding this 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/classes.py
Outdated
| If true, `decision_function_shape`='ovr', and number of classes > 2, | ||
| `predict` will break ties according to the confidence values of | ||
| `decision_function`. Please note that breaking ties comes | ||
| at a relatively high computational cost compared to a simple predict. |
There was a problem hiding this comment.
the practical utility from this explanation may not be clear to everybody
There was a problem hiding this comment.
also, documentation of the default=False case is missing
sklearn/svm/classes.py
Outdated
| Deprecated *decision_function_shape='ovo' and None*. | ||
|
|
||
| break_ties : bool, optional (default=False) | ||
| If true, `decision_function_shape`='ovr', and number of classes > 2, |
There was a problem hiding this comment.
the practical utility from this explanation may not be clear to everybody
There was a problem hiding this comment.
We can put in a very common use case that a wide spectrum of data analysts are likely to have encountered in some shape or form.
There was a problem hiding this comment.
those details are suitable for .rst documentations, not the docstrings.
There was a problem hiding this comment.
Actually, I must confess that I am unsure what exactly the benefits of turning this on is. Let me explain what I think I have understood that the benefit is, and maybe it can help the docstring:
"Using confidence to break ties can lead to a better prediction for this specific instances".
Is that correct? If so, I would suggest that this sentence is added before "Please note that..."
|
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 |
|
@jnothman I increased the plot size a bit, I hope that helps. |
|
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. |
|
@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 |
|
@NicolasHug does this look good now? |
NicolasHug
left a comment
There was a problem hiding this 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?
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. |
|
@NicolasHug done :) |
NicolasHug
left a comment
There was a problem hiding this comment.
Last comment, LGTM otherwise!
|
Thanks Adrin! |
Fixes #8277
This is a proposal to fix the issue in
SVCandNuSVCwhere the predict does not break ties but decision_function does, whendecision_function_shape='ovr' and n_classes > 2The 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
Falseto keep it backward compatible, and if needed, in the future, switch the default value toTruein an appropriate deprecation cycle.