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

MNT Deprecate unused parameter from OneClassSVM fit method. #20843

Merged

Conversation

jmloyola
Copy link
Member

@jmloyola jmloyola commented Aug 26, 2021

Reference Issues/PRs

Addresses a comment given in this PR.

What does this implement/fix? Explain your changes.

This PR removes deprecates the parameter params 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).

Copy link
Member

@thomasjpfan thomasjpfan left a 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

Copy link
Member

@ogrisel ogrisel left a 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.

@jmloyola
Copy link
Member Author

@ogrisel, @thomasjpfan, upon further inspection, I'm not that certain that the removal of params should go through the normal deprecation cycle (raise FutureWarning if used), since anybody using an extra keyword parameter in the fit method now receives an error.
For example, adding the parameter extra_param as follows:

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:

TypeError: fit() got an unexpected keyword argument 'extra_param'

To my knowledge, there is no extra keyword parameter that BaseLibSVM's fit method could accept. A couple of years ago the parameter class_weight was present with a deprecation message. This commit removed it, but param remained in OneClassSVM. Thus, the current status has been like this for a few years.

Please, let me know what do you think. Should we continue with the deprecation cycle or not?

@glemaitre
Copy link
Member

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.

Would we not consider this as a bug fix instead. For instance, a small typo in sample_weight could lead to something unexpected: estimator.fit(X, y, sample_weights=some_weights) while removing the kwargs would raise the right error.

@glemaitre
Copy link
Member

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.

@@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def fit(self, X, y=None, sample_weight=None):
def fit(self, X, y=None, sample_weight=None, **params):

sklearn/svm/_classes.py Show resolved Hide resolved
@jmloyola jmloyola changed the title MNT Remove unused parameter from OneClassSVM fit method. MNT Deprecate unused parameter from OneClassSVM fit method. Sep 4, 2021
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

small nitpicks. LGTM

sklearn/svm/_classes.py Outdated Show resolved Hide resolved
sklearn/svm/_classes.py Outdated Show resolved Hide resolved
doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved
jmloyola and others added 3 commits September 6, 2021 08:28
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>
@jmloyola
Copy link
Member Author

jmloyola commented Sep 6, 2021

Thanks for the suggestions @glemaitre 🤓

@glemaitre glemaitre merged commit b43f057 into scikit-learn:main Sep 6, 2021
@jmloyola jmloyola deleted the remove_unused_param_oneclasssvm branch September 6, 2021 12:47
adrinjalali pushed a commit that referenced this pull request Sep 7, 2021
Co-authored-by: glemaitre <g.lemaitre58@gmail.com>
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 10, 2021
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 10, 2021
…_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
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
ivannz added a commit to ivannz/scikit-learn that referenced this pull request May 15, 2022
…_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
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jun 14, 2022
…_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
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Aug 29, 2022
…_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
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Aug 29, 2022
…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)
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Sep 5, 2022
…_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
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Sep 5, 2022
…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)
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.

4 participants