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

DEP start to raise warning with inconsistent combination of hyperparameters in SVM #19630

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PGijsbers
Copy link
Contributor

References #19614.

What does this implement/fix? Explain your changes.

Gives a FutureWarning when a user configures a LibSVM learner with 'linear' kernel but also non-default gamma.

from sklearn.datasets import load_iris
from sklearn.svm import SVC

x, y = load_iris(return_X_y=True)
clf = SVC(kernel='linear', gamma=1e-6)
clf.fit(x, y)
print(clf.score(x, y))

previous output:

0.9933333333333333

current output:

...\scikit-learn\sklearn\svm\_base.py:220: FutureWarning: Setting 'gamma' when using 'linear' kernel may raise a `ValueError` starting in version 1.1 (renaming of 0.26).
  warnings.warn(
SVC(gamma=1e-06, kernel='linear')
0.9933333333333333

Any other comments?

Went with a FutureWarning instead of the DeprecationWarning as discussed in the issue, because:

  • DeprecationWarning is ignored by default (unless it's code from __main__),
  • It does not remove old functionality (setting gamma with kernel='linear' had no function).

I tried to find guidelines for this in the dev docs, but it either isn't there or I couldn't find it.
Just let me know if I should revert it to DeprecationWarning.
Also wasn't sure if a note of this should be added to the docs, since they already describe which parameter affects which kernel.

Tests ran with Windows/Py3.8:

pytest sklearn/svm
pytest doc/modules/svm.rst
pytest sklearn/tests/test_common.py -k SVC

@PGijsbers
Copy link
Contributor Author

Let me know if I should add a changelog entry. I don't see a reference (e.g. here, so I don't know what determines if a changelog entry is needed.

if self.kernel == 'linear':
warnings.warn(
"Setting 'gamma' when using 'linear' kernel may raise a "
"`ValueError` starting in version 1.1 (renaming of 0.26).",
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the usual cycle: if we start warning in 1.0, we should start erroring in 1.2 (n+0.2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer, I wasn't familiar with the usual cycle so I assumed it would be n+0.1 :) I'll start making changes when a consensus is reached in #19614.

@glemaitre glemaitre changed the title Warn when gamma is set with linear kernel (#19614) DEP start to raise warning with inconsistent combination of hyperparameters in SVM Jul 29, 2021
@glemaitre
Copy link
Member

Before to review, I would like that we settle on what do for all parameters combination. It will be discussed in the original issue #19614

@glemaitre glemaitre added the Needs Decision Requires decision label Jul 29, 2021
@adrinjalali
Copy link
Member

The issue seems to have moved forward a bit, I think we can move forward in this PR @PGijsbers

@PGijsbers
Copy link
Contributor Author

While I would still be very happy to contribute, I am very busy right now, so it might take a while for me to get to this (likely more than a month). If anyone else wants to take this, that is good with me. If not, then I should come around to it eventually.

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

Successfully merging this pull request may close these issues.

None yet

4 participants