-
-
Notifications
You must be signed in to change notification settings - Fork 25k
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 Completes position arg deprecation #17272
MNT Completes position arg deprecation #17272
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.
LGTM if green
Thanks Thomas. I'd make args positional in these functions as well: [<function sklearn._config.set_config(*, assume_finite=None, working_memory=None, print_changed_only=None, display=None)>,
<function sklearn.feature_extraction.image.extract_patches(arr, *, patch_shape=8, extraction_step=1)>,
<function sklearn.metrics._regression.max_error(*, y_true, y_pred)>,
<function sklearn.metrics.cluster._supervised.adjusted_rand_score(*, labels_true, labels_pred)>,
<function sklearn.metrics.cluster._supervised.homogeneity_score(*, labels_true, labels_pred)>,
<function sklearn.metrics.cluster._supervised.completeness_score(*, labels_true, labels_pred)>,
<function sklearn.metrics.pairwise.laplacian_kernel(X, Y=None, *, gamma=None)>,
<function sklearn.metrics.pairwise.chi2_kernel(X, Y=None, *, gamma=1.0)>,
<function sklearn.metrics.pairwise.linear_kernel(X, Y=None, *, dense_output=True)>,
<function sklearn.metrics.pairwise.polynomial_kernel(X, Y=None, *, degree=3, gamma=None, coef0=1)>,
<function sklearn.metrics.pairwise.cosine_similarity(X, Y=None, *, dense_output=True)>,
<function sklearn.metrics.pairwise.rbf_kernel(X, Y=None, *, gamma=None)>,
<function sklearn.metrics.pairwise.sigmoid_kernel(X, Y=None, *, gamma=None, coef0=1)>,
<function sklearn.model_selection._search.fit_grid_point(X, y, estimator, parameters, *, train, test, scorer, verbose, error_score=nan, **fit_params)>,
<function sklearn.neural_network._base.log_loss(*, y_true, y_prob)>,
<function sklearn.utils.safe_indexing(X, indices, *, axis=0)>,
<function sklearn.utils.class_weight.compute_class_weight(*, class_weight, classes, y)>,
<function sklearn.utils.extmath.safe_sparse_dot(a, b, *, dense_output=False)>,
<function sklearn.utils.extmath.randomized_svd(M, n_components, *, n_oversamples=10, n_iter='auto', power_iteration_normalizer='auto', transpose='auto', flip_sign=True, random_state=0)>,
<function sklearn.utils.extmath.randomized_range_finder(A, *, size, n_iter, power_iteration_normalizer='auto', random_state=None)>,
<function sklearn.utils.extmath.weighted_mode(a, w, *, axis=0)>,
<function sklearn.utils.graph.single_source_shortest_path_length(graph, source, *, cutoff=None)>,
<function sklearn.utils.sparsefuncs.incr_mean_variance_axis(X, *, axis, last_mean, last_var, last_n)>,
<function sklearn.utils.sparsefuncs.mean_variance_axis(X, *, axis)>,
<function sklearn.utils.sparsefuncs.inplace_row_scale(X, *, scale)>,
<function sklearn.utils.sparsefuncs.inplace_column_scale(X, *, scale)>] WDYT? I do find |
tests are also failing. |
I am +0.25 on this, because I think there would be too many warnings with this change. We already released 0.23 version where we left |
WDYT of adding a "version" parameter to |
What do you mean by "those"? I think the deprecations in this PR should still have a 2 version deprecation cycle. |
Sorry, by "those" I meant |
Ah I see. I can add version to the decorator. We can open an issue for making |
IMO we shouldn't make these kwonly considering we kept y_true and y_pred positional for all the other metrics.
I sort of agree but the good news is we're always consistent on this. In the end that's just a convention users need to get used to, just like they get used to passing |
I thought it'd be an easy sell, but apparently @NicolasHug doesn't think so :P So I'm +1 on the issue and talking about it in the call. |
WDYT of the other suggestions I have in #17272 (comment)? (kinda have a feeling we may have discussed some of them in other PRs) |
Here are the ones I left out from #17272 (comment)
|
I guess |
I agree with you. But it seems there are too many places internally where we call it positionally :/ |
sklearn/cluster/_dbscan.py
Outdated
algorithm='auto', leaf_size=30, p=2, sample_weight=None, | ||
n_jobs=None): | ||
@_deprecate_positional_args | ||
def dbscan(X, *, eps=0.5, min_samples=5, metric='minkowski', |
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.
DBSCAN
accepts positional eps
sklearn/covariance/_graph_lasso.py
Outdated
|
||
def graphical_lasso(emp_cov, alpha, cov_init=None, mode='cd', tol=1e-4, | ||
@_deprecate_positional_args | ||
def graphical_lasso(emp_cov, *, alpha, cov_init=None, mode='cd', tol=1e-4, |
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.
GraphicalLasso
accepts positional alpha
sklearn/metrics/pairwise.py
Outdated
@@ -977,7 +977,8 @@ def paired_distances(X, Y, *, metric="euclidean", **kwds): | |||
|
|||
|
|||
# Kernels | |||
def linear_kernel(X, Y=None, dense_output=True): | |||
@_deprecate_positional_args | |||
def linear_kernel(X, Y=None, *, dense_output=True): |
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 think in the PR I touched this file, we decided not to touch the kernels, but I'm happy with this.
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'll leave it alone.
@@ -273,7 +273,8 @@ def _determine_key_type(key, accept_slice=True): | |||
# TODO: remove in 0.24 | |||
@deprecated("safe_indexing is deprecated in version " | |||
"0.22 and will be removed in version 0.24.") | |||
def safe_indexing(X, indices, axis=0): | |||
@_deprecate_positional_args | |||
def safe_indexing(X, indices, *, axis=0): |
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.
we're deprecating this func anyway.
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.
yeah I already have a PR that removes it
I went with |
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.
LGTM when green. Are you fine with the changes @NicolasHug ?
@@ -71,7 +71,7 @@ def test_estimate_bandwidth_with_sparse_matrix(): | |||
# Test estimate_bandwidth with sparse matrix | |||
X = sparse.lil_matrix((1000, 1000)) | |||
msg = "A sparse matrix was passed, but dense data is required." | |||
assert_raise_message(TypeError, msg, estimate_bandwidth, X, 200) | |||
assert_raise_message(TypeError, msg, estimate_bandwidth, X) |
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.
This is setting quantile
to 200
which does not make sense for this check.
yes |
* ENH Adds public functions with position args warning * BUG Circlar import * STY * BUG Add keywords * BUG Fixes warnings * ENH Adds other public functions * CLN Suggestion * CLN Suggestion * REV Revert pairwise * ENH Positoinal args for compute_class_weight
* ENH Adds public functions with position args warning * BUG Circlar import * STY * BUG Add keywords * BUG Fixes warnings * ENH Adds other public functions * CLN Suggestion * CLN Suggestion * REV Revert pairwise * ENH Positoinal args for compute_class_weight
* ENH Adds public functions with position args warning * BUG Circlar import * STY * BUG Add keywords * BUG Fixes warnings * ENH Adds other public functions * CLN Suggestion * CLN Suggestion * REV Revert pairwise * ENH Positoinal args for compute_class_weight
After this PR here are the public functions that have 2 or more arguments that do not have a
*
for keyword only arguments. In this case, "public function" means it is listed inclasses.rst
.These functions are not updated with this PR:
The
utils
andpairwise
ones are the biggest bunch that do not have*
.Are there any other functions we want to add
*
to?