API Deprecate positional arguments in preprocessing by thomasjpfan · Pull Request #16996 · scikit-learn/scikit-learn

thomasjpfan · 2020-04-22T02:51:32Z

Reference Issues/PRs

adrinjalali

adrinjalali · 2020-04-22T10:31:26Z

sklearn/preprocessing/_data.py


-    def __init__(self, feature_range=(0, 1), copy=True):
+    @_deprecate_positional_args
+    def __init__(self, *, feature_range=(0, 1), copy=True):


I'm happy with feature_range positional or not.

Positional isn't ambiguous I would say so +1 for that

adrinjalali · 2020-04-22T10:32:32Z

sklearn/preprocessing/_data.py


-def normalize(X, norm='l2', axis=1, copy=True, return_norm=False):
+@_deprecate_positional_args
+def normalize(X, *, norm='l2', axis=1, copy=True, return_norm=False):


I'd put norm positional if we're doing positional norm in Normalizer?

yes, also the names ("l2", etc) are descriptive enough

adrinjalali · 2020-04-22T10:34:13Z

sklearn/preprocessing/_label.py


-def label_binarize(y, classes, neg_label=0, pos_label=1, sparse_output=False):
+@_deprecate_positional_args
+def label_binarize(y, classes, *, neg_label=0, pos_label=1,


should we be consistent with classes here and MultiLabelBinarizer?

Went with making classes positional for now (since it is a smaller diff). But I am okay with either.

Hmm I think I prefer using classes as a keyword only option. Lets see what it looks like.

jnothman · 2020-04-22T12:58:18Z

sklearn/preprocessing/_data.py


-def power_transform(X, method='yeo-johnson', standardize=True, copy=True):
+@_deprecate_positional_args
+def power_transform(X, *, method='yeo-johnson', standardize=True, copy=True):


You've not decorated the class version of this. I'm happy for methods to be positional.,..?

Updated PR with the class version of this.

I'm happy for methods to be positional.,..?

I am also okay with keeping methods positional. We kind of started with making functions positional in the earlier PRs, are we okay with continuing with this?

NicolasHug

nits but LGTM anyway

NicolasHug · 2020-04-22T12:55:17Z

sklearn/preprocessing/_data.py


-def normalize(X, norm='l2', axis=1, copy=True, return_norm=False):
+@_deprecate_positional_args
+def normalize(X, *, norm='l2', axis=1, copy=True, return_norm=False):


yes, also the names ("l2", etc) are descriptive enough

NicolasHug · 2020-04-22T12:56:47Z

sklearn/preprocessing/_data.py


-    def __init__(self, n_quantiles=1000, output_distribution='uniform',
+    @_deprecate_positional_args
+    def __init__(self, n_quantiles=1000, *, output_distribution='uniform',


n_quantiles is kwonly in quantile_transform (arguably because of the axis param that comes before). Should it be kwonly here too?

Yea its because of the axis. I'll make it keyword only here as well.

NicolasHug · 2020-04-22T12:57:47Z

sklearn/preprocessing/_data.py


-def power_transform(X, method='yeo-johnson', standardize=True, copy=True):
+@_deprecate_positional_args
+def power_transform(X, *, method='yeo-johnson', standardize=True, copy=True):


here again the names 'yeo-johnson' and 'boxcox' are non-ambiguous and would lead me to just not force a kwonly but no strong opinion

NicolasHug · 2020-04-22T12:58:48Z

sklearn/preprocessing/_function_transformer.py

-    def __init__(self, func=None, inverse_func=None, validate=False,
+
+    @_deprecate_positional_args
+    def __init__(self, func=None, *, inverse_func=None, validate=False,


Suggested change

def __init__(self, func=None, *, inverse_func=None, validate=False,

def __init__(self, func=None, inverse_func=None, *, validate=False,

I assume usage would be non-ambiguous

NicolasHug · 2020-04-22T13:00:27Z

sklearn/preprocessing/_data.py


-    def __init__(self, feature_range=(0, 1), copy=True):
+    @_deprecate_positional_args
+    def __init__(self, *, feature_range=(0, 1), copy=True):


Positional isn't ambiguous I would say so +1 for that

adrinjalali

Otherwise LGTM.

adrinjalali · 2020-04-23T07:59:59Z

sklearn/preprocessing/_data.py


-def minmax_scale(X, feature_range=(0, 1), axis=0, copy=True):
+@_deprecate_positional_args
+def minmax_scale(X, *, feature_range=(0, 1), axis=0, copy=True):


should this follow the MinMaxScaler and have feature_range positional?

* API Deprecate positional arguments in preprocessing * CLN Address comments * CLN Uses classes as a keyword only argument * BUG Fix

API Deprecate positional arguments in preprocessing

49e3bd1

thomasjpfan added this to the 0.23 milestone Apr 22, 2020

github-actions bot added the module:preprocessing label Apr 22, 2020

adrinjalali reviewed Apr 22, 2020

View reviewed changes

jnothman reviewed Apr 22, 2020

View reviewed changes

NicolasHug approved these changes Apr 22, 2020

View reviewed changes

thomasjpfan added 2 commits April 22, 2020 18:46

CLN Address comments

e66506e

CLN Uses classes as a keyword only argument

f617271

adrinjalali approved these changes Apr 23, 2020

View reviewed changes

BUG Fix

c7e9795

adrinjalali merged commit e54cd3c into scikit-learn:master Apr 23, 2020

	def __init__(self, func=None, *, inverse_func=None, validate=False,
	def __init__(self, func=None, inverse_func=None, *, validate=False,

Uh oh!

Conversation

thomasjpfan commented Apr 22, 2020

Reference Issues/PRs

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants