API Deprecate positional arguments in svm module #16973
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adrinjalali
approved these changes
Apr 21, 2020
| @@ -627,7 +629,8 @@ class SVC(BaseSVC): | |||
|
|
|||
| _impl = 'c_svc' | |||
|
|
|||
| def __init__(self, C=1.0, kernel='rbf', degree=3, gamma='scale', | |||
| @_deprecate_positional_args | |||
| def __init__(self, *, C=1.0, kernel='rbf', degree=3, gamma='scale', | |||
There was a problem hiding this comment.
I would leave C and nu positional, I think, but no strong preference.
There was a problem hiding this comment.
I think not. Too unclear with SVC(0.5)
jnothman
reviewed
Apr 21, 2020
| @@ -627,7 +629,8 @@ class SVC(BaseSVC): | |||
|
|
|||
| _impl = 'c_svc' | |||
|
|
|||
| def __init__(self, C=1.0, kernel='rbf', degree=3, gamma='scale', | |||
| @_deprecate_positional_args | |||
| def __init__(self, *, C=1.0, kernel='rbf', degree=3, gamma='scale', | |||
There was a problem hiding this comment.
I think not. Too unclear with SVC(0.5)
| @@ -992,7 +996,8 @@ class SVR(RegressorMixin, BaseLibSVM): | |||
|
|
|||
| _impl = 'epsilon_svr' | |||
|
|
|||
| def __init__(self, kernel='rbf', degree=3, gamma='scale', | |||
| @_deprecate_positional_args | |||
| def __init__(self, *, kernel='rbf', degree=3, gamma='scale', | |||
There was a problem hiding this comment.
I'd consider kernel being positional in SVMs... but SVC has C in the way.
There was a problem hiding this comment.
Maybe that's something we can do once the deprecation cycle is over, because there would be no risk of breaking user code?
NicolasHug
approved these changes
Apr 22, 2020
sklearn/svm/_classes.py
Outdated
| def __init__(self, penalty='l2', loss='squared_hinge', dual=True, tol=1e-4, | ||
| C=1.0, multi_class='ovr', fit_intercept=True, | ||
| @_deprecate_positional_args | ||
| def __init__(self, penalty='l2', *, loss='squared_hinge', dual=True, |
There was a problem hiding this comment.
any reason to make penalty positional but not the loss?
I feel like both are equally unambiguous given the possible names
| @@ -992,7 +996,8 @@ class SVR(RegressorMixin, BaseLibSVM): | |||
|
|
|||
| _impl = 'epsilon_svr' | |||
|
|
|||
| def __init__(self, kernel='rbf', degree=3, gamma='scale', | |||
| @_deprecate_positional_args | |||
| def __init__(self, *, kernel='rbf', degree=3, gamma='scale', | |||
There was a problem hiding this comment.
Maybe that's something we can do once the deprecation cycle is over, because there would be no risk of breaking user code?
gio8tisu
pushed a commit
to gio8tisu/scikit-learn
that referenced
this pull request
May 15, 2020
* API Keyword only for svm * BUG Fix * CLN Address comments
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
May 30, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
May 30, 2020
viclafargue
pushed a commit
to viclafargue/scikit-learn
that referenced
this pull request
Jun 26, 2020
* API Keyword only for svm * BUG Fix * CLN Address comments
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Jul 5, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Jul 5, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Aug 5, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Oct 15, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Nov 28, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Dec 12, 2020
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Feb 25, 2021
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Feb 25, 2021
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Mar 27, 2021
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Jun 15, 2021
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Jul 23, 2021
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
May 15, 2022
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Jun 14, 2022
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Aug 29, 2022
ivannz
added a commit
to ivannz/scikit-learn
that referenced
this pull request
Sep 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Towards #15005
CC @adrinjalali