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+2] EHN: Allow specifying arbitrary kernel for semi-supervised learning. #5762

Merged
merged 5 commits into from Sep 21, 2016

Conversation

Projects
None yet
5 participants
@musically-ut
Contributor

musically-ut commented Nov 8, 2015

I needed a custom kernel for a research project I am working on (for calculating euclidean distance between cities based on longitude/latitude).

If there is interest in including these changes, I will be willing to polish the code further.

String identifier for kernel function to use.
kernel : {'knn', 'rbf', function}
String identifier for kernel function to use or the kernel function to calculate
the [n_samples, n_samples] sized weight matrix.

This comment has been minimized.

@glouppe

glouppe Nov 15, 2015

Member

Can you be more explicit about the expected input and output values of the user-provided function?

This comment has been minimized.

@musically-ut

musically-ut Nov 19, 2015

Contributor

I have changed the documentation to talk about the input/output of the kernel function. If there is interest in having this functionality, I will fix the documentation for the LabelSpread and BaseLabelPropagation class as well for completeness. However, I will give coming up with a convincing test case for #5774 more priority.

@amueller

This comment has been minimized.

Member

amueller commented Sep 14, 2016

LGTM

@amueller amueller changed the title from EHN: Allow specifying arbitrary kernel for semi-supervised learning. to [MRG + 1] EHN: Allow specifying arbitrary kernel for semi-supervised learning. Sep 14, 2016

@amueller

This comment has been minimized.

Member

amueller commented Sep 14, 2016

Needs an entry in whatsnew.rst (for 0.19 I guess)

@musically-ut

This comment has been minimized.

Contributor

musically-ut commented Sep 20, 2016

Are we waiting on something here?

~
ut

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 20, 2016

You need a second review, and a what's new entry..

@@ -82,7 +82,7 @@ class BaseLabelPropagation(six.with_metaclass(ABCMeta, BaseEstimator,
----------
kernel : {'knn', 'rbf'}

This comment has been minimized.

@jnothman

jnothman Sep 20, 2016

Member

and callable now?

kernel : {'knn', 'rbf', function}
String identifier for kernel function to use or the kernel function
itself. Only 'rbf' and 'knn' strings are valid inputs. The function
passed should take two inputs, each of size [n_samples, n_features],

This comment has been minimized.

@jnothman

jnothman Sep 20, 2016

Member

"size" -> "shape"

String identifier for kernel function to use or the kernel function
itself. Only 'rbf' and 'knn' strings are valid inputs. The function
passed should take two inputs, each of size [n_samples, n_features],
and return a [n_samples, n_samples] sized weight matrix.

This comment has been minimized.

@jnothman
@jnothman

This comment has been minimized.

Member

jnothman commented Sep 20, 2016

Otherwise LGTM

@musically-ut

This comment has been minimized.

Contributor

musically-ut commented Sep 20, 2016

Oops.

The underlying documentation had some changes. I am rebasing this PR off the current master and will push my changes in a while. I'll also quash the commits together while I am at it. I am not sure how that plays with the outdated diffs on Github, though.

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 20, 2016

No need to squash commits.

On 20 September 2016 at 22:39, Utkarsh Upadhyay notifications@github.com
wrote:

Oops.

The underlying documentation had some changes. I am rebasing this PR off
the current master and will push my changes in a while. I'll also quash the
commits together while I am at it. I am not sure how that plays with the
outdated diffs on Github, though.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5762 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz6_WH9-9Iz27cYGYg4xTMe3m0SXpjks5qr9QRgaJpZM4GeKI8
.

@musically-ut

This comment has been minimized.

Contributor

musically-ut commented Sep 20, 2016

I'll fix the flake8 failures on the tests.

I wonder why they didn't trigger before; perhaps the flake8 test was extended to tests recently?

@NelleV

This comment has been minimized.

Member

NelleV commented Sep 21, 2016

Yeah! Thanks for the patch. This is going to be useful for me :)

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 21, 2016

LGTM. Please add an entry to whats_new.rst

@jnothman jnothman changed the title from [MRG + 1] EHN: Allow specifying arbitrary kernel for semi-supervised learning. to [MRG+2] EHN: Allow specifying arbitrary kernel for semi-supervised learning. Sep 21, 2016

@musically-ut

This comment has been minimized.

Contributor

musically-ut commented Sep 21, 2016

Done.

@jnothman jnothman merged commit 070093e into scikit-learn:master Sep 21, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnothman

This comment has been minimized.

Member

jnothman commented Sep 21, 2016

Note @ogrisel and @amueller, this currently has what's new under 0.18. If it is not backported, what's new should be moved.

@amueller

This comment has been minimized.

Member

amueller commented Sep 22, 2016

argh... this is a new feature and should not be backported....

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 22, 2016

It's not a very risky feature, so...

TomDLT added a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment