API kwonly for neighbors module#17004
Conversation
|
|
||
| def __init__(self, n_neighbors=5, radius=1.0, | ||
| @_deprecate_positional_args | ||
| def __init__(self, *, n_neighbors=5, radius=1.0, |
There was a problem hiding this comment.
this one is a bit special because it has both n_neighbors and radius so I make both kwonly but feel free to disagree
sklearn/neighbors/_graph.py
Outdated
| """ | ||
| def __init__(self, mode='distance', n_neighbors=5, algorithm='auto', | ||
| @_deprecate_positional_args | ||
| def __init__(self, mode='distance', n_neighbors=5, *, algorithm='auto', |
There was a problem hiding this comment.
I'd leave no positional here. KNeighborsTransformer('distance', 5) doesn't really read well.
There was a problem hiding this comment.
Yeah, we probably should have put n_neighbors first.
sklearn/neighbors/_graph.py
Outdated
| """ | ||
| def __init__(self, mode='distance', radius=1., algorithm='auto', | ||
| @_deprecate_positional_args | ||
| def __init__(self, mode='distance', radius=1., *, algorithm='auto', |
thomasjpfan
left a comment
There was a problem hiding this comment.
Looks good aside from the comments on the *NeighborsTransformers.
sklearn/neighbors/_graph.py
Outdated
| """ | ||
| def __init__(self, mode='distance', n_neighbors=5, algorithm='auto', | ||
| @_deprecate_positional_args | ||
| def __init__(self, mode='distance', n_neighbors=5, *, algorithm='auto', |
There was a problem hiding this comment.
Yeah, we probably should have put n_neighbors first.
|
tests still failing |
|
This one was weird... tests were all green before except for codecov, then after I merged master I got 30 legit errors because of some kwonly args. No idea why they didn't show up before... Looks like everything is green now, it's +2 so I'll merge before it goes bad Thanks for the reviews |
Towards #15005