API make __init__ params in compose module kw-only#16542
API make __init__ params in compose module kw-only#16542amueller merged 4 commits intoscikit-learn:masterfrom
Conversation
jnothman
left a comment
There was a problem hiding this comment.
Are we checking for warnings being raised in examples (and tests) by these changes?
sklearn/compose/_target.py
Outdated
| """ | ||
| def __init__(self, regressor=None, transformer=None, | ||
| @_deprecate_positional_args | ||
| def __init__(self, *, regressor=None, transformer=None, |
There was a problem hiding this comment.
Hmm. I think I'm okay with the regressor being specified positionally.
There was a problem hiding this comment.
First I thought so too, but then I realized there's a regressor and a transformer, and they kinda are of the same nature, and it'd be easy to confuse which one comes first. That's why I put it before regressor.
There was a problem hiding this comment.
hm I would also think after regressor is better. transformer is optional and I have actually never used it. The main application of this for me is the log transform and we don't have a LogTransformer right now.
Other than that looks good.
We fail on our |
|
@amueller happy with this one now? |
|
gentle ping @amueller |
|
Thanks! |
* API make __init__ params in compose module kw-only * pep8 * move * to after
* API make __init__ params in compose module kw-only * pep8 * move * to after
this should be an easy one
ping @NicolasHug @rth