[MRG+1] allow callable kernels in cross-validation #8005

Merged
merged 4 commits into from Dec 20, 2016

Conversation

Projects
None yet
3 participants
@amueller
Member

amueller commented Dec 7, 2016

This allows callable kernels in cross-validation.
This was forbidden in #649 but I don't understand why.
I kept it in #803 when I introduced _pairwise for some reason.
Discussion in #7930 where @jnothman and me got kinda confused I think.

Removing the condition in metaestimators.py does not break any tests.

I checked all other occurrences of _pairwise and it only returns True for "precomputed", never for callable.s

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 7, 2016

Member

This seems more sensible... but I'd like to hear whether there are reasons other than computational cost that others can find for this restriction existing in the first place.

Member

jnothman commented Dec 7, 2016

This seems more sensible... but I'd like to hear whether there are reasons other than computational cost that others can find for this restriction existing in the first place.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 7, 2016

Member

In short, LGTM

Member

jnothman commented Dec 7, 2016

In short, LGTM

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 7, 2016

Member

weird I didn't get the error locally... must have not run the test properly..

Member

amueller commented Dec 7, 2016

weird I didn't get the error locally... must have not run the test properly..

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 7, 2016

Member

I have no idea why I put _safe_split into utils.meta_estimator, but I don't really have a better idea now.

Member

amueller commented Dec 7, 2016

I have no idea why I put _safe_split into utils.meta_estimator, but I don't really have a better idea now.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 8, 2016

Member
Member

jnothman commented Dec 8, 2016

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 8, 2016

Member
Member

jnothman commented Dec 8, 2016

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 14, 2016

Member

(I wonder whether kallable kernels were outlawed at some point when joblib didn't check for picklability and things crashed??)

Member

jnothman commented Dec 14, 2016

(I wonder whether kallable kernels were outlawed at some point when joblib didn't check for picklability and things crashed??)

@jnothman jnothman changed the title from [MRG] allow callable kernels in cross-validation to [MRG+1] allow callable kernels in cross-validation Dec 14, 2016

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 14, 2016

Member

for reference,

from sklearn.model_selection import cross_val_score
from sklearn.svm import SVC
from sklearn.datasets import make_blobs
import numpy as np

X, y = make_blobs()
cross_val_score(SVC(kernel=lambda x, y: np.inner(x, y)), X, y)

works, n_jobs=2 crashes with a pickling error - unsurprisingly.

Member

amueller commented Dec 14, 2016

for reference,

from sklearn.model_selection import cross_val_score
from sklearn.svm import SVC
from sklearn.datasets import make_blobs
import numpy as np

X, y = make_blobs()
cross_val_score(SVC(kernel=lambda x, y: np.inner(x, y)), X, y)

works, n_jobs=2 crashes with a pickling error - unsurprisingly.

@tguillemot

This comment has been minimized.

Show comment
Hide comment
@tguillemot

tguillemot Dec 16, 2016

Contributor

Thanks for the exemple @amueller.
LGTM

Contributor

tguillemot commented Dec 16, 2016

Thanks for the exemple @amueller.
LGTM

@tguillemot

This comment has been minimized.

Show comment
Hide comment
@tguillemot

tguillemot Dec 16, 2016

Contributor

Maybe we can add something about that in what's new ?

Contributor

tguillemot commented Dec 16, 2016

Maybe we can add something about that in what's new ?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 19, 2016

Member

this is good for merge, right?

Member

amueller commented Dec 19, 2016

this is good for merge, right?

@jnothman jnothman merged commit 5d0c7f5 into scikit-learn:master Dec 20, 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

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

@Przemo10 Przemo10 referenced this pull request Mar 17, 2017

Closed

update fork (#1) #8606

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

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

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

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

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