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

Don't hard-code kernel parameters in Nystroem #5229

Closed
amueller opened this Issue Sep 8, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@amueller
Member

amueller commented Sep 8, 2015

The current Nystroem kernel approximation interface is bad as it assumes some default parameters for certain kernels. It should get a dictionary of kernel_args instead, I think.
See #5211. This needs a deprecation.

@khalibartan

This comment has been minimized.

Show comment
Hide comment
@khalibartan

khalibartan Sep 13, 2015

I would like to take up the issue if no one is assigned.

khalibartan commented Sep 13, 2015

I would like to take up the issue if no one is assigned.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Sep 13, 2015

Member

go ahead.

Member

amueller commented Sep 13, 2015

go ahead.

@khalibartan

This comment has been minimized.

Show comment
Hide comment
@khalibartan

khalibartan Sep 14, 2015

Actually I'm new to the community , so if you could help me out.
I was reading the metrics.pairwise script and I found that parameters for different kernels also have default value (similar to that in nystroem).
So do I need to raise type error if certain parameters for specific kernel are not given by user (for example like gamma for rbf kernel), because if user doesn't give the value then default value given to arguments in pairwise script will be used.So enhancement will be not be useful.

Also shouldn't we get arguments through dictionaries for different kernels in pairwise script .

khalibartan commented Sep 14, 2015

Actually I'm new to the community , so if you could help me out.
I was reading the metrics.pairwise script and I found that parameters for different kernels also have default value (similar to that in nystroem).
So do I need to raise type error if certain parameters for specific kernel are not given by user (for example like gamma for rbf kernel), because if user doesn't give the value then default value given to arguments in pairwise script will be used.So enhancement will be not be useful.

Also shouldn't we get arguments through dictionaries for different kernels in pairwise script .

@carrillo

This comment has been minimized.

Show comment
Hide comment
@carrillo

carrillo Sep 18, 2015

Contributor

Dear Utkarsh,

did you make any progress? If not, do you mind me taking over?

Contributor

carrillo commented Sep 18, 2015

Dear Utkarsh,

did you make any progress? If not, do you mind me taking over?

@khalibartan

This comment has been minimized.

Show comment
Hide comment
@khalibartan

khalibartan Sep 19, 2015

Actually I have done what I have written above , I'm just waiting for him to respond. Whenever @amueller responds I'll send a pull request

khalibartan commented Sep 19, 2015

Actually I have done what I have written above , I'm just waiting for him to respond. Whenever @amueller responds I'll send a pull request

carrillo added a commit to carrillo/scikit-learn that referenced this issue Sep 26, 2015

Addressed issue #5229: Don't hard-code kernel parameters in Nystroem
1. Generated a class variable: KERNEL_DEFAULT_PARAMS dictionary of kernel default parameters.
2. Modified __init__(): Set default values for kernel to Null
3. Modified _get_kernel_parameters(): if the self.kernel parameter is not callable, it checks
if the kernel is contained in the dictionary. For each kernel parameter defined in the dictionary it
assigns the default if None specified in __init__() or the one specified.
@carrillo

This comment has been minimized.

Show comment
Hide comment
@carrillo

carrillo Sep 26, 2015

Contributor

I have the fix laying around for some time now. I'll make a pull request in a moment. Please ignore this request, if I'm acting out of line. I just want to get this off my desk.

Contributor

carrillo commented Sep 26, 2015

I have the fix laying around for some time now. I'll make a pull request in a moment. Please ignore this request, if I'm acting out of line. I just want to get this off my desk.

@raghavrv

This comment has been minimized.

Show comment
Hide comment
@raghavrv

raghavrv Oct 20, 2015

Member

@carrillo Have you raised a PR for this?

Member

raghavrv commented Oct 20, 2015

@carrillo Have you raised a PR for this?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 20, 2015

Member

@khalibartan yeah that is the fix that I was thinking off.

Member

amueller commented Oct 20, 2015

@khalibartan yeah that is the fix that I was thinking off.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 20, 2015

Member

@carrillo's pr is here: #5316

Member

amueller commented Oct 20, 2015

@carrillo's pr is here: #5316

@carrillo

This comment has been minimized.

Show comment
Hide comment
@carrillo

carrillo Oct 20, 2015

Contributor

Sorry, I haven't updated the PR yet. If you have fixed it, go ahead. I won't be able to work on it within the next week or so...

Contributor

carrillo commented Oct 20, 2015

Sorry, I haven't updated the PR yet. If you have fixed it, go ahead. I won't be able to work on it within the next week or so...

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