API kwonly args in manifold, metrics, mixture, model_selection, multclass, multioutput#16982
API kwonly args in manifold, metrics, mixture, model_selection, multclass, multioutput#16982NicolasHug merged 10 commits intoscikit-learn:masterfrom
Conversation
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @adrinjalali
Made comments but LGTM when green anyway
I'm fine with groups being kwonly. It might make things easier if one day we want to integrate them into fit_params
| # It's okay to evaluate regression metrics on classification too | ||
| mse_scorer = check_scoring(est, 'neg_mean_squared_error') | ||
| r2_scorer = check_scoring(est, 'r2') | ||
| mse_scorer = check_scoring(est, scoring='neg_mean_squared_error') |
There was a problem hiding this comment.
this is another case that seems a bit verbose
sklearn/metrics/_plot/roc_curve.py
Outdated
|
|
||
| def __init__(self, fpr, tpr, roc_auc, estimator_name): | ||
| @_deprecate_positional_args | ||
| def __init__(self, *, fpr, tpr, roc_auc, estimator_name): |
There was a problem hiding this comment.
I'd agree that putting * after precision and recall would maybe be ok since the class is called PrecisionRecall..., but here there's no way to know which one's supposed to come first.
There was a problem hiding this comment.
Actually, not sure. Because the order it comes first in corresponds to roc_curve output, so it should be easy to use RocCurveDisplay(*roc_curve_output, ...)
There was a problem hiding this comment.
Yes, but not sure how many people would write it as you suggest, and it probably looks a bit cryptic to most people.
sklearn/model_selection/_split.py
Outdated
|
|
||
| def split(self, X, y=None, groups=None): | ||
| @_deprecate_positional_args | ||
| def split(self, X, y=None, *, groups=None): |
There was a problem hiding this comment.
Not sure how valuable it is to make groups kwonly in a method that currently always takes <=3 params
There was a problem hiding this comment.
My thinking was that very often the third argument after X, y is sample_weight, and this removes that ambiguity.
There was a problem hiding this comment.
On the other hand, it seems at least we internally heavily pass groups as a positional arg. So I guess that itself is a verdict that it's breaking too much code. I'll revert this one.
|
Failures |
|
I think it's green now? Can't really tell what it's complaining about. |
sklearn/metrics/_plot/roc_curve.py
Outdated
|
|
||
| def __init__(self, fpr, tpr, roc_auc, estimator_name): | ||
| @_deprecate_positional_args | ||
| def __init__(self, *, fpr, tpr, roc_auc, estimator_name): |
There was a problem hiding this comment.
Actually, not sure. Because the order it comes first in corresponds to roc_curve output, so it should be easy to use RocCurveDisplay(*roc_curve_output, ...)
|
GitHub's Approve button is making errors for me. I approve. |
|
Thanks @adrinjalali |
Towards #15005
Making
groupsmay be controversial, but I find it sometimes ambiguous when we could have eithersample_weightorgroups. That made the diff larger than I intended.Our CI is probably gonna fail on the docs here, but before I go ahead with the rest of it, it'd be nice to have some feedback.
ping @jnothman @thomasjpfan @NicolasHug