Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors #182

Merged
merged 7 commits into from Nov 2, 2016

Conversation

@glemaitre
Copy link
Member

@glemaitre glemaitre commented Oct 30, 2016

No description provided.

@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Oct 30, 2016

Address issue #171

@coveralls
Copy link

@coveralls coveralls commented Oct 30, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.826% when pulling d54e396 on glemaitre:neighbours_object into ad355b7 on scikit-learn-contrib:master.

@glemaitre glemaitre force-pushed the glemaitre:neighbours_object branch from d54e396 to 61050d2 Oct 30, 2016
@coveralls
Copy link

@coveralls coveralls commented Oct 30, 2016

Coverage Status

Coverage decreased (-0.3%) to 98.794% when pulling 61050d2 on glemaitre:neighbours_object into ad355b7 on scikit-learn-contrib:master.

@coveralls
Copy link

@coveralls coveralls commented Oct 30, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.803% when pulling 172f786 on glemaitre:neighbours_object into ad355b7 on scikit-learn-contrib:master.

@glemaitre glemaitre changed the title [WIP] Add the option to pass a KNeighborsMixin instead of simple n_neigbors [MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors Oct 30, 2016
@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Oct 30, 2016

@dvro @chkoar Good to be reviewed

@coveralls
Copy link

@coveralls coveralls commented Oct 30, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.803% when pulling eef0ee9 on glemaitre:neighbours_object into ad355b7 on scikit-learn-contrib:master.

@glemaitre glemaitre changed the title [MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors [WIP] Add the option to pass a KNeighborsMixin instead of simple n_neigbors Oct 31, 2016
@dvro
Copy link
Member

@dvro dvro commented Oct 31, 2016

@glemaitre, first at all, good job! This will be useful for techniques that use adaptive distances (such as undersampling methods like ATISA).

Now, if we want to allow a user to pass a NearestNeighbors object, we should allow him to pass a RadiusNeighborsClassifier, and this object doesn't have a kneighbors method, but a radius_neighbors method.

Something like this should do the work:

if hasattr(self.nn_, 'kneighbors'):
    nnhood_idx = self.nn_.kneighbors(sub_samples_x, return_distance=False)[:, 1:]
elif hasattr(self.nn_, 'radius_neighbors'):
    nnhood_idx = self.nn_.radius_neighbors(sub_samples_x, return_distance=False)[:, 1:]
else:
    raise Exception
@chkoar
Copy link
Member

@chkoar chkoar commented Oct 31, 2016

Good catch @dvro.

@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Oct 31, 2016

I am not against it. However, I see an issue coming using metric distance.
In the case of a RadiusNeighborsMixin class, the number of neighbours returned might be not always the same which might make fail most of the algorithm internally.

I would split this for another PR in which we ensure that the algorithm can handle such specificity.

@chkoar @dvro any thoughts?

@glemaitre glemaitre changed the title [WIP] Add the option to pass a KNeighborsMixin instead of simple n_neigbors [MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors Oct 31, 2016
@glemaitre glemaitre changed the title [MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors [WIP] Add the option to pass a KNeighborsMixin instead of simple n_neigbors Oct 31, 2016
@glemaitre glemaitre force-pushed the glemaitre:neighbours_object branch from eef0ee9 to 1b9f6c6 Oct 31, 2016
@coveralls
Copy link

@coveralls coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.754% when pulling 1b9f6c6 on glemaitre:neighbours_object into 70f2e75 on scikit-learn-contrib:master.

@coveralls
Copy link

@coveralls coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.762% when pulling 304c873 on glemaitre:neighbours_object into 70f2e75 on scikit-learn-contrib:master.

@dvro
Copy link
Member

@dvro dvro commented Oct 31, 2016

@glemaitre @chkoar makes sense! We can create another issue for this case!
[MRG]

@glemaitre glemaitre changed the title [WIP] Add the option to pass a KNeighborsMixin instead of simple n_neigbors [MRG] Add the option to pass a KNeighborsMixin instead of simple n_neigbors Oct 31, 2016
@coveralls
Copy link

@coveralls coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.777% when pulling f09cb3e on glemaitre:neighbours_object into 70f2e75 on scikit-learn-contrib:master.

@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Oct 31, 2016

@chkoar your turn ;)

@@ -41,6 +42,10 @@ class SMOTE(BaseBinarySampler):
k_neighbors : int, optional (default=5)

This comment has been minimized.

@chkoar

chkoar Nov 1, 2016
Member

I would say:
k_neighbors : int or object, optional (default=5)
If int, the number of the nearest neighbours to be used to construct synthetic samples.
If object, an estimator that inherits from sklearn.neighbors.base.KNeighborsMixin that will be used to find the k_neighbors.

@glemaitre
Copy link
Member Author

@glemaitre glemaitre commented Nov 2, 2016

@chkoar I updated all the different doc

@coveralls
Copy link

@coveralls coveralls commented Nov 2, 2016

Coverage Status

Coverage decreased (-0.2%) to 98.777% when pulling d0ad1cf on glemaitre:neighbours_object into 70f2e75 on scikit-learn-contrib:master.

@chkoar chkoar merged commit 4889c2e into scikit-learn-contrib:master Nov 2, 2016
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
code-quality/landscape Code quality increased by 0.06%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 98.777%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants