[MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn #109

chkoar · 2016-07-25T16:21:21Z

For consistency reasons I think that we should follow scikit-learn conventions in naming the parameters.
I propose to change the size_ngh parameter to n_neighbors. Unfortunately, this change will have impact in the public API. It is an early modification but it will break users code. I don't know if we could merge this change without a deprecation warning.

dvro · 2016-07-25T18:32:24Z

@chkoar we could leave size_ngh and include a warning, allowing n_neighbors in a kwargs.

IMO, the user would pass either the n_neighbors parameter
but we also could allow someone to pass the NearestNeighbors object so that they could
use customized decision metrics, radius, n_neighbors, variable n_neighbors based on cluster
analysis (...) that way, we keep it clean but also allowing users to use complex neighborhood
cleansing.

Thoughts, @glemaitre ?

glemaitre · 2016-07-25T22:22:14Z

@dvro I think that the solution with kwargs could be used. We could announce for the next release the change in the API.

Regarding passing the estimator itself, we can think about it and what it is implying on the API design. My only concern is that you are giving a lot of freedom to the user and at the end, getting maybe far from the spirit of the original research paper.

What do you think?

chkoar · 2016-07-25T22:48:34Z

https://github.com/scikit-learn/scikit-learn/blob/51a765a/sklearn/base.py#L162

All estimators should specify all the parameters that can be set
at the class level in their __init__ as explicit keyword
arguments (no *args or **kwargs).

glemaitre · 2016-07-26T08:02:11Z

So I don't see other solutions than depreciation cycle such that it will change in version 0.4

glemaitre · 2016-07-31T23:26:45Z

@chkoar @dvro What is the state of that one. Shall we announce the deprecation?

chkoar · 2016-08-01T10:30:26Z

@glemaitre @dvro yes. We could work with **kwargs for n_neighbors for a version and announce a deprecation.

dvro · 2016-08-01T20:24:23Z

I agree!

glemaitre · 2016-08-01T20:34:34Z

@chkoar we let you that one?

chkoar · 2016-08-01T20:36:03Z

Yeap. Actually, we have to consider removing all occurrences of **kwargs also.

glemaitre · 2016-08-01T20:37:57Z

Yep I am working on the testing now, and so one of them in CenterClustering. Usually we used it when the user would like to pass some parameters for the classifier used.

chkoar · 2016-08-17T10:49:53Z

Replying to @dvro regarding the NearestNeighbors object.

We could have the n_neighbors parameter expecting an integer value in order to construct the NearestNeighbors object with reasonable default values. We could accept also a NearestNeighbors object instead of an integer value with all the parameters provided by the user. With this approach we could get rid of the **kwargs and let the user to provide his/her parameters for the NearestNeighbors estimator.

chkoar · 2016-08-30T11:02:08Z

ping @glemaitre @dvro

glemaitre · 2016-08-30T11:55:52Z

We could have the n_neighbors parameter expecting an integer value in order to construct the NearestNeighbors object with reasonable default values. We could accept also a NearestNeighbors object instead of an integer value with all the parameters provided by the user. With this approach we could get rid of the **kwargs and let the user to provide his/her parameters for the NearestNeighbors estimator.

It seems the best solution for me. At least, the back-compatibility is ensured.

chkoar · 2016-08-30T12:05:06Z

Ok, great. I'll make the changes at the first chance.

glemaitre · 2016-10-25T10:14:53Z

@chkoar Do you want me to get on that one?

chkoar · 2016-10-25T11:08:31Z

No problem. You can contribute in my branch or you create a new one and work there.

As you said we must provide both parameters in the constructor

Announce the deprecation
n_neighbors should be None as default (or vice versa)
internally we'll use n_neighbors
if both size_ngh and n_neighbors are present we should raise an exception (if this doesn't brake the check_estimator test) or a warning

What do you think @glemaitre ?

glemaitre · 2016-10-25T15:05:18Z

I think that I already a fork of yours so I will go for that. All you points are correct and I will address them.

chkoar · 2016-10-25T16:00:08Z

@glemaitre great. There is a need for a rebase, though. So, rebase with the upstream, force push and I will accept all changes.

glemaitre · 2016-10-25T16:01:25Z

@chkoar ok I am going for it now

chkoar · 2016-10-25T16:09:26Z

@glemaitre There is already a pending PR in chkoar#3

glemaitre · 2016-10-25T16:12:59Z

Yep I found that back in my branches ;)

…h scikit-learn.

coveralls · 2016-10-25T23:39:12Z

Coverage decreased (-0.06%) to 98.943% when pulling fc2d1c6 on chkoar:n_neighbors into db85c84 on scikit-learn-contrib:master.

glemaitre · 2016-10-26T07:28:13Z

@dvro @chkoar LGTM for merging

chkoar · 2016-10-26T08:55:04Z

@dvro this PR needs a sweet squash and merge commit

coveralls · 2016-10-26T08:55:10Z

Coverage decreased (-0.06%) to 98.943% when pulling a52fd28 on chkoar:n_neighbors into db85c84 on scikit-learn-contrib:master.

…ency with scikit-learn (scikit-learn-contrib#109) * Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn. * Implement deprecation for smote_enn and enn * Add the changes in documentation * Make the changes in the base function * Minor comment fixes

glemaitre changed the title ~~[MRG] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn~~ [WIP] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn Jul 26, 2016

glemaitre added this to the 0.2.alpha milestone Sep 1, 2016

chkoar and others added 3 commits October 25, 2016 18:57

Rename all occurrences of size_ngh to n_neighbors for consistency wit…

a8e811b

…h scikit-learn.

Implement deprecation for smote_enn and enn

24fb3b2

Add the changes in documentation

05599c1

glemaitre added the Hacktoberfest label Oct 25, 2016

chkoar mentioned this pull request Oct 25, 2016

Remove **kwargs #168

Closed

Make the changes in the base function

fc2d1c6

chkoar force-pushed the n_neighbors branch from d820807 to fc2d1c6 Compare October 25, 2016 23:36

glemaitre changed the title ~~[WIP] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn~~ [MRG] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn Oct 26, 2016

Minor comment fixes

a52fd28

glemaitre changed the title ~~[MRG] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn~~ [MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn Oct 26, 2016

massich approved these changes Oct 26, 2016

View reviewed changes

glemaitre merged commit 3ee7147 into scikit-learn-contrib:master Oct 26, 2016

chkoar deleted the n_neighbors branch February 3, 2017 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn #109

[MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn #109

chkoar commented Jul 25, 2016 •

edited

Loading

dvro commented Jul 25, 2016

glemaitre commented Jul 25, 2016

chkoar commented Jul 25, 2016

glemaitre commented Jul 26, 2016 •

edited

Loading

glemaitre commented Jul 31, 2016

chkoar commented Aug 1, 2016

dvro commented Aug 1, 2016

glemaitre commented Aug 1, 2016

chkoar commented Aug 1, 2016 •

edited

Loading

glemaitre commented Aug 1, 2016

chkoar commented Aug 17, 2016 •

edited

Loading

chkoar commented Aug 30, 2016

glemaitre commented Aug 30, 2016

chkoar commented Aug 30, 2016

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016 •

edited

Loading

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016

glemaitre commented Oct 25, 2016

coveralls commented Oct 25, 2016 •

edited

Loading

glemaitre commented Oct 26, 2016

chkoar commented Oct 26, 2016 •

edited

Loading

coveralls commented Oct 26, 2016 •

edited

Loading

[MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn #109

[MRG+1] Rename all occurrences of size_ngh to n_neighbors for consistency with scikit-learn #109

Conversation

chkoar commented Jul 25, 2016 • edited Loading

dvro commented Jul 25, 2016

glemaitre commented Jul 25, 2016

chkoar commented Jul 25, 2016

glemaitre commented Jul 26, 2016 • edited Loading

glemaitre commented Jul 31, 2016

chkoar commented Aug 1, 2016

dvro commented Aug 1, 2016

glemaitre commented Aug 1, 2016

chkoar commented Aug 1, 2016 • edited Loading

glemaitre commented Aug 1, 2016

chkoar commented Aug 17, 2016 • edited Loading

chkoar commented Aug 30, 2016

glemaitre commented Aug 30, 2016

chkoar commented Aug 30, 2016

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016 • edited Loading

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016

glemaitre commented Oct 25, 2016

chkoar commented Oct 25, 2016

glemaitre commented Oct 25, 2016

coveralls commented Oct 25, 2016 • edited Loading

glemaitre commented Oct 26, 2016

chkoar commented Oct 26, 2016 • edited Loading

coveralls commented Oct 26, 2016 • edited Loading

chkoar commented Jul 25, 2016 •

edited

Loading

glemaitre commented Jul 26, 2016 •

edited

Loading

chkoar commented Aug 1, 2016 •

edited

Loading

chkoar commented Aug 17, 2016 •

edited

Loading

chkoar commented Oct 25, 2016 •

edited

Loading

coveralls commented Oct 25, 2016 •

edited

Loading

chkoar commented Oct 26, 2016 •

edited

Loading

coveralls commented Oct 26, 2016 •

edited

Loading