Implement SLEP009: keyword-only arguments

[jnothman]

SLEP009 is all but accepted.

It proposes to make most parameters keyword-only.

We should do this by first:

• Merging ENH Add Deprecating Position Arguments Helper #13311
• Perhaps getting some stats on usage of positional arguments as per SLEP009: keyword only arguments enhancement_proposals#19 (comment)
• applying the deprecation to each subpackage. Checked means PR opened at least.
  • base
  • calibration
  • cluster
  • compose
  • covariance
  • cross_decomposition
  • datasets
  • decomposition
  • discriminant_analysis
  • dummy
  • ensemble
  • feature_extraction
  • feature_selection
  • gaussian_process
  • impute
  • inspection
  • isotonic
  • kernel_approximation
  • kernel_ridge
  • linear_model
  • manifold
  • metrics
  • metrics.pairwise
  • mixture
  • model_selection
  • multiclass
  • multioutput
  • naive_bayes
  • neighbors
  • neural_network
  • pipeline
  • preprocessing
  • random_projection
  • semi_supervised
  • svm
  • tree
  • utils

We might along the way establish rules of thumb and principles like "are the semantics reasonably clear when the argument is passed positionally?" As I noted on the mailing list, I think they are clear for PCA's components, for Pipeline's steps, and for GridSearchCV's estimator and parameter grid. Other parameters of those estimators seem more suitable for keyword-only. Trickier is whether n_components in TSNE should follow PCA in being positional... It's not as commonly set by users.

[adrinjalali]

Realistically, this should be in 0.23. We already have too much on our plate, I think.

[jnothman]

Hmmmm... I don't see why, if the release target is November.

[qinhanmin2014]

I think they are clear for PCA's components, for Pipeline's steps, and for GridSearchCV's estimator and parameter grid.

I'm wondering whether it's easy to decide which patameters should be positional, e.g., do we think PCA(2) is reasonable? At least I don't like it.

And how shall we make the decision? I guess we don't want to open a vote for every class/function, right? So is +2 enough, or do we need +3?

[rth]

e.g., do we think PCA(2) is reasonable? At least I don't like it.

It's about leaving some of this to the user discretion, or forcing them to use what we think best. Yes, PCA(2) is bad, while PCA(n_components) is reasonable. At least I wouldn't object to a PR using it, would you? Users can resent a limitation of their freedom (when to use or not position args) in cases when what there is no overwhelming reason for it.

So is +2 enough, or do we need +3?

I would say +2 since the SLEP was accepted but wait at bit before merging to give the possibility for feedback?

[qinhanmin2014]

At least I wouldn't object to a PR using it, would you?

I don't like it but I'm not opposed to it.

Users can resent a limitation of their freedom (when to use or not position args) in cases when what there is no overwhelming reason for it.

Hmm, not sure, but the SLEP is passed?

[rth]

At least I wouldn't object to a PR using it, would you?

I don't like it but I'm not opposed to it.

Let's put in another way. As a user, imagine currently I have a few 1000 lines of perfectly fine code that uses PCA(n_components) (and other comparable use cases). If tomorrow it's going to start raising warnings to change it to PCA(n_components=n_components) and require me to do maintenance work without good reason, personally I would be unhappy and will complain about it to whatever project did that.

[jnothman]

There's no reason to have a higher standard of review here than any other deprecation, after the SLEP is accepted. The SLEP would have been accepted if Andy was around.

[amueller]

@agramfort explicitly argued for accepting PCA(n_components).

I really think the point made on the mailing list about allowing users to have clear expectations is important. If we can't write down a simple rule it's hard for users to have clear expectations.

Doing some quick stats:

from collections import Counter
from inspect import signature
from sklearn.utils.testing import all_estimators

counts = Counter()

for name, est in all_estimators():
    sig = signature(est)
    if len(sig.parameters):
        first = list(sig.parameters.keys())[0]
        counts[first] += 1

There's 61 different first arguments in our estimators.

Details

{'n_components': 25, 'alpha': 12, 'estimator': 11, 'base_estimator': 8, 'kernel': 8, 'n_clusters': 7, 'store_precision': 6, 'n_estimators': 6, 'n_neighbors': 6, 'loss': 6, 'score_func': 6, 'alphas': 5, 'fit_intercept': 5, 'criterion': 5, 'C': 3, 'eps': 3, 'input': 3, 'copy': 3, 'threshold': 3, 'penalty': 3, 'missing_values': 3, 'bandwidth': 2, 'priors': 2, 'l1_ratio': 2, 'strategy': 2, 'epsilon': 2, 'estimators': 2, 'n_bins': 2, 'n_iter': 2, 'hidden_layer_sizes': 2, 'categories': 2, 'nu': 2, 'radius': 2, 'norm': 2, 'skewedness': 1, 'with_centering': 1, 'damping': 1, 'dtype': 1, 'sample_steps': 1, 'gamma': 1, 'check_y': 1, 'transformers': 1, 'n_quantiles': 1, 'dictionary': 1, 'method': 1, 'degree': 1, 'neg_label': 1, 'steps': 1, 'patch_size': 1, 'solver': 1, 'n_features': 1, 'transformer_list': 1, 'func': 1, 'min_samples': 1, 'metric': 1, 'classes': 1, 'feature_range': 1, 'Cs': 1, 'y_min': 1, 'regressor': 1, 'n_nonzero_coefs': 1}

Maybe having a white-list of those that we allow would be useful?
Say, 'n_components', 'alpha', 'estimator', 'base_estimator', 'kernel' (this is not for SVC), 'n_clusters', 'n_estimators', 'n_neighbors', 'C', 'steps', 'regressor', 'transformers'?

Though the C is a bit of an outlier and I think having 'store_precision' be positional would not be very useful so I didn't list it. Generally I think for all meta-estimators the first argument should be positional.

[thomasjpfan]

Generally I think for all meta-estimators the first argument should be positional.

Agreed.

I prefer narrowing the list down to just clustering, decomposition, and meta estimator parameters: 'n_components', 'estimator', 'base_estimator', 'n_clusters', 'n_neighbors', 'steps', 'regressor', 'transformers'

[amueller]

Hm that's deprecating LogisticRegression(0.01) and RandomForestClassifier(100)`` ... I'm not super opposed but also could see some resistance?

[jnothman]

LogisticRegression(0.01) isn't a thing: the first parameter is penalty

[jnothman]

I might try get someone to help tackle this (either trying to gather the usage statistics, or to put the APIs into a form where all can be considered) over the next couple of days. Is someone else working on it?

[amueller]

A major company recently did a giant github scrape and analyzed sklearn usage. I asked them whether they can share their results.