-
Notifications
You must be signed in to change notification settings - Fork 10
Replace utility classes with sklearn classes in NB4 #30
Conversation
rosecers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make similar changes as in #29 and re-request review.
rosecers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still argue that we should be using a precomputed kernel across the notebook. Recomputing the kernel with every model will greatly slow down the notebook.
| "from utilities.kernels import linear_kernel, gaussian_kernel, center_kernel\n", | ||
| "from utilities.classes import KPCA, KRR, SparseKPCA, SparseKRR\n", | ||
| "from utilities.classes import KPCovR, SparseKPCovR\n", | ||
| "from utilities.kernels import linear_kernel, gaussian_kernel, center_kernel # to be replaced with functions from skcosmo\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete this if it is not used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are used somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adapt this like you did in #29. There shouldn't be any kernel-tutorials kernel functions in this notebook. You should use sklearn kernels and centerers from skcosmo (scikit-learn-contrib/scikit-matter#8)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
| "kernel_func = gaussian_kernel\n", | ||
| "kernel_type = 'gaussian'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these used anywhere? If not, delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "# Implementation in `skcosmo`\n", | ||
| "\n", | ||
| "Classes from the utility module enable computing SparseKPCA, SparseKRR, and SparseKPCovR with a scikit.learn-like syntax. `SparseKPCovR` is located in `utilities/kpcovr.py`." | ||
| "Classes from the skcosmo module enable computing SparseKPCA, SparseKRR, and SparseKPCovR with a scikit.learn-like syntax. `SparseKPCovR` is located in `skcosmo/PCovR/KPCovR.py`." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Co-authored-by: rosecers <47536110+rosecers@users.noreply.github.com>
| "ref_kpca.fit(X_train, K=K_train)\n", | ||
| "\n", | ||
| "table_from_dict([ref_kpca.statistics(X_train, K=K_train), \n", | ||
| "ref_kpca = KernelPCA(n_components=n_PC, kernel='rbf',gamma = 1.0, fit_inverse_transform=True)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment on line 364
rosecers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two outstanding questions with respect to kernel definitions.
| "kernel_params = {\"kernel\": \"rbf\", \"gamma\": 1.0}\n", | ||
| "kernel_func = gaussian_kernel\n", | ||
| "kernel_params = {\"kernel\": \"precomputed\"}\n", | ||
| "kernel_func = rbf_kernel\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use functools to define
from functools import partial
kernel_func = partial(rbf_kernel, gamma=1.0)
to avoid having to repeat gamma everywhere below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! It works!
rosecers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure this runs clean then feel free to merge



Replace some utility classes in
4_SparseKernelMethods.ipynbwith appropriatesklearnclasses and modify the corresponding text.