-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
MNT Deprecate unused parameter from OneClassSVM fit method. #20843
MNT Deprecate unused parameter from OneClassSVM fit method. #20843
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @jmloyola !
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I theory we should go through a deprecation cycle for this change. That means raising a warning to warn that the fit method will no longer accept these useless parameters in version 1.2.
@ogrisel, @thomasjpfan, upon further inspection, I'm not that certain that the removal of from sklearn import svm
X = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1]]
clf = svm.OneClassSVM()
clf.fit(X, extra_param='unused') The following error is raised:
To my knowledge, there is no extra keyword parameter that BaseLibSVM's fit method could accept. A couple of years ago the parameter Please, let me know what do you think. Should we continue with the deprecation cycle or not? |
Would we not consider this as a bug fix instead. For instance, a small typo in |
After discussing with @ogrisel IRL, I am fine with the deprecation cycle. If a user makes a mistake in the spelling, he/she will at least get the warning that something is wrong. |
sklearn/svm/_classes.py
Outdated
@@ -1543,7 +1543,7 @@ def __init__( | |||
random_state=None, | |||
) | |||
|
|||
def fit(self, X, y=None, sample_weight=None, **params): | |||
def fit(self, X, y=None, sample_weight=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def fit(self, X, y=None, sample_weight=None): | |
def fit(self, X, y=None, sample_weight=None, **params): |
Co-authored-by: glemaitre <g.lemaitre58@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nitpicks. LGTM
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Thanks for the suggestions @glemaitre 🤓 |
Co-authored-by: glemaitre <g.lemaitre58@gmail.com>
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787)
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787) uncompromisingly reformat plot_oneclass_vs_svdd with black
…earn#20843) Co-authored-by: glemaitre <g.lemaitre58@gmail.com>
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787) uncompromisingly reformat plot_oneclass_vs_svdd with black
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787) uncompromisingly reformat plot_oneclass_vs_svdd with black
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787) uncompromisingly reformat plot_oneclass_vs_svdd with black
…cSVM scikit-learn#24001) finish v1.2 deprecation of params kwargs in `.fit` of SVDD (similar to ocSVM scikit-learn#20843) removed SVDD param-validation exception from test_common.py since scikit-learn#23462 is go (scikit-learn#22722)
…_new add backticks (scikit-learn#20914), deprecate **params in fit (scikit-learn#20843), add feature_names_in_ (scikit-learn#20787) uncompromisingly reformat plot_oneclass_vs_svdd with black
…cSVM scikit-learn#24001) finish v1.2 deprecation of params kwargs in `.fit` of SVDD (similar to ocSVM scikit-learn#20843) TST ensure SVDD passes param-validation test_common.py due to scikit-learn#23462 (scikit-learn#22722)
Reference Issues/PRs
Addresses a comment given in this PR.
What does this implement/fix? Explain your changes.
This PR
removesdeprecates the parameterparams
from OneClassSVM's fit method since the BaseLibSVM's fit method does not have any more parameters than the already passed as keywords (X, y, sample_weight).