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] FIX Nystroem with precomputed kernel #14706
Conversation
LGTM
Please also add a bugfix entry in doc/whats_new/0.22.rst
.
actually, not sure if this is the right fix, maybe |
Now we can use the |
@amueller if self.kernel == 'precomputed':
return params
if not callable(self.kernel):
for param in (KERNEL_PARAMS[self.kernel]):
if getattr(self, param) is not None:
params[param] = getattr(self, param)
else:
if (self.gamma is not None or
self.coef0 is not None or
self.degree is not None):
raise ValueError("Don't pass gamma, coef0 or degree to "
"Nystroem if using a callable kernel.") |
Right, if someone wants to iterate over |
Please correct me If I am wrong, (self.gamma is not None or
self.coef0 is not None or
self.degree is not None) when
On a side note, I saw here, we have been using this inside 'precomputed': None, # HACK: precomputed is always allowed, never called That is the reason, why I had pursued ahead to add |
I feel if we can avoid the hack that's better ;) |
ya, makes sense. Thanks for the input. Co-Authored-By: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
Co-Authored-By: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
Please also test that the correct error is raised with a precomputed kernel and passed kernel parameters.
Thanks a lot @adrinjalali for your valuable comments. Please review my latest commit. |
Nice, thanks ! |
Reference Issues/PRs
Fixes #14641
What does this implement/fix? Explain your changes.
Added
precomputed
inKERNEL_PARAMS
Nystroem
withprecomputed
kernel usingpolynomial_kernel
Any other comments?