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

added missing chi2 gamma parameter #5211

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@nrhine1

nrhine1 commented Sep 3, 2015

chi2 kernel gamma parameter was missing in KERNEL_PARAMS dictionary, which caused the passed gamma to be ignored when using filter_params = True in pairwise_kernels(X, filter_params = True, metric = 'chi2', gamma = gamma)

@@ -1215,7 +1215,7 @@ def kernel_metrics():
KERNEL_PARAMS = {
"additive_chi2": (),
"chi2": (),
"chi2": frozenset(["gamma"]),
"cosine": (),
"exp_chi2": frozenset(["gamma"]),

This comment has been minimized.

@amueller

amueller Sep 8, 2015

Member

can you remove the exp_chi2? It's not in PAIRWISE_KERNEL_FUNCTIONS and I seem to have left it in there by mistake.

@amueller

amueller Sep 8, 2015

Member

can you remove the exp_chi2? It's not in PAIRWISE_KERNEL_FUNCTIONS and I seem to have left it in there by mistake.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Sep 8, 2015

Member

Travis is failing.

Member

amueller commented Sep 8, 2015

Travis is failing.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Sep 8, 2015

Member

Hm, the travis failure is because the default gamma for rbf is None and the default for chi squared is 1.
Maybe the API for Nystroem is not that great.
Maybe it should have been a dictionary of kernel args instead of explicitly hard-coding some kernel parameters, see #5229.

Maybe just add gamma to the dictionary if the entry is not None?

Member

amueller commented Sep 8, 2015

Hm, the travis failure is because the default gamma for rbf is None and the default for chi squared is 1.
Maybe the API for Nystroem is not that great.
Maybe it should have been a dictionary of kernel args instead of explicitly hard-coding some kernel parameters, see #5229.

Maybe just add gamma to the dictionary if the entry is not None?

@rohit12

This comment has been minimized.

Show comment
Hide comment
@rohit12

rohit12 Sep 13, 2015

I would like to solve this issue. Where can I get some background about it? Also, is it a simple matter of removing the exp_chi2 line?

rohit12 commented Sep 13, 2015

I would like to solve this issue. Where can I get some background about it? Also, is it a simple matter of removing the exp_chi2 line?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Sep 13, 2015

Member

The issue is mostly #5229 but can be worked around here as I explained above. Removing exp_chi2 is just cosmetic.

Member

amueller commented Sep 13, 2015

The issue is mostly #5229 but can be worked around here as I explained above. Removing exp_chi2 is just cosmetic.

@carrillo

This comment has been minimized.

Show comment
Hide comment
@carrillo

carrillo Sep 28, 2015

Contributor

I fixed the problem with the hardcoded kernel parameter in the Nystroem approximation in PR #5316. In order to work with the changes addressed here, one would have to also add the gamma parameter to the kernel_default_param dictionary. This line is currently commented out in the PR.

Contributor

carrillo commented Sep 28, 2015

I fixed the problem with the hardcoded kernel parameter in the Nystroem approximation in PR #5316. In order to work with the changes addressed here, one would have to also add the gamma parameter to the kernel_default_param dictionary. This line is currently commented out in the PR.

@mth4saurabh mth4saurabh referenced this pull request Jan 9, 2016

Closed

[MRG] removed hard coded kernel params in Nystroem #6145

2 of 2 tasks complete

@amueller amueller referenced this pull request Jun 19, 2017

Merged

[MRG +1] Nystroem params #9166

@ogrisel ogrisel closed this in #9166 Jun 30, 2017

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