Skip to content

added missing chi2 gamma parameter#5211

Closed
nrhinehart wants to merge 1 commit intoscikit-learn:masterfrom
nrhinehart:chi2paramfix
Closed

added missing chi2 gamma parameter#5211
nrhinehart wants to merge 1 commit intoscikit-learn:masterfrom
nrhinehart:chi2paramfix

Conversation

@nrhinehart
Copy link
Copy Markdown

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

amueller commented Sep 8, 2015

Travis is failing.

@amueller
Copy link
Copy Markdown
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
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

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

@carrillo
Copy link
Copy Markdown
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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants