Skip to content
This repository was archived by the owner on Jan 15, 2026. It is now read-only.

Replace utility classes with sklearn classes in NB3#29

Merged
rosecers merged 6 commits into
lab-cosmo:masterfrom
PKGuo:sklearnify_NB3
Nov 24, 2020
Merged

Replace utility classes with sklearn classes in NB3#29
rosecers merged 6 commits into
lab-cosmo:masterfrom
PKGuo:sklearnify_NB3

Conversation

@PKGuo

@PKGuo PKGuo commented Nov 18, 2020

Copy link
Copy Markdown
Contributor

Replace some utility classes in 3_KernelMethods.ipynb with appropriate sklearn classes and modify the corresponding text.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment on lines +1187 to +1188
"T_KPCA_train = kpca.transform(X_train)\n",
"T_KPCA_test= kpca.transform(X_test)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see above

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

@PKGuo PKGuo Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not that I forgot to deal with this comment, but I don't think using kernel_params = dict (kernel= "precomputed") is a good idea, and supplying kpca.transform with x_train is not an error that needs to be fixed immediately. I plan to deal with it after getting your reply.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
"outputs": [],
"source": [
"krr = KRR(regularization=regularization, kernel_type=kernel_type)"
"krr = KernelRidge(alpha=regularization, kernel = 'rbf', gamma = 1.0)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
"outputs": [],
"source": [
"krr.fit(X_train, Y_train, K=K_train)"
"krr.fit(X_train, Y_train)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

@PKGuo PKGuo Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not that I forgot to deal with this comment, but I don't think using kernel_params = dict (kernel= "precomputed") is a good idea, and supplying krr.fit with x_train is not an error that needs to be fixed immediately. I plan to deal with it after getting your reply.

"Y_krr_test = krr.transform(X_test, K=K_test)"
"Y_krr_train = krr.predict(X_train)\n",
"Y_krr_test = krr.predict(X_test)"
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

@PKGuo PKGuo Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What am I supposed to "see above"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

Comment thread notebooks/3_KernelMethods.ipynb Outdated
" krr.statistics(X=X_test, Y=Y_test, K=K_test)], \n",
" headers = [\"Training\", \"Testing\"], \n",
" title=\"Kernel Ridge Regression\")"
"from utilities.classes import KPCovR # to be replaced with scalers from skcosmo"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

scalers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It should be "to be replaced with the class from skcosmo"

@PKGuo PKGuo requested a review from rosecers November 18, 2020 15:39
@PKGuo PKGuo changed the title Sklearnify NB3 Replace utility classes with sklearn classes in NB3 Nov 18, 2020
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment thread notebooks/3_KernelMethods.ipynb Outdated
"outputs": [],
"source": [
"kpca.fit(X_train, K=K_train)"
"kpca.fit(X_train)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

then use kernel_params = dict(kernel="precomputed")!

@rosecers

rosecers commented Nov 20, 2020 via email

Copy link
Copy Markdown
Contributor

@PKGuo PKGuo requested a review from rosecers November 20, 2020 15:32
Comment thread notebooks/3_KernelMethods.ipynb Outdated
"from utilities.kernels import linear_kernel, gaussian_kernel, center_kernel\n",
"from utilities.classes import PCA, LR, KPCA, KRR, KPCovR\n",
"\n",
"from utilities.kernels import linear_kernel, gaussian_kernel,center_kernel # to be replaced with functions from skcosmo\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we don't use the kernel-tutorials kernels, please delete this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment on lines +136 to +140
"K_train_raw = kernel_func(X_train, X_train)"
"K_train_raw = kernel_func(X_train, X_train, gamma = 1.0)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you center these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will be centered later.
image

Comment thread notebooks/3_KernelMethods.ipynb Outdated
"outputs": [],
"source": [
"kpca.fit(X_train, K=K_train)"
"kpca.fit(X_train)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's not only about time, but about RAM. We have to assume that these notebooks will be used in a pedagogic setting, where students will be using computers of a variety of capabilities. It may be fast on your computer, but if we know it's a potential time-sink, we may as well avoid it.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
Comment on lines +1187 to +1188
"T_KPCA_train = kpca.transform(X_train)\n",
"T_KPCA_test= kpca.transform(X_test)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

Comment thread notebooks/3_KernelMethods.ipynb Outdated
"outputs": [],
"source": [
"krr.fit(X_train, Y_train, K=K_train)"
"krr.fit(X_train, Y_train)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

"Y_krr_test = krr.transform(X_test, K=K_test)"
"Y_krr_train = krr.predict(X_train)\n",
"Y_krr_test = krr.predict(X_test)"
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still needs to be fixed.

@PKGuo PKGuo requested a review from rosecers November 22, 2020 04:44

@rosecers rosecers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Almost ready to go!

Comment thread notebooks/3_KernelMethods.ipynb Outdated
"# The string variable is used to pass to the utility classes. \n",
"kernel_func = gaussian_kernel\n",
"kernel_params = {\"kernel\": \"precomputed\"}\n",
"kernel_func = rbf_kernel\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as in #30, you can use functools to avoid having to include gamma everywhere

from functools import partial

kernel_func = partial(rbf_kernel, gamma=1.0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you! It works.

"Y_krr_test = krr.transform(X_test, K=K_test)"
"Y_krr_train = krr.predict(X_train)\n",
"Y_krr_test = krr.predict(X_test)"
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What am I supposed to "see above"?

@PKGuo PKGuo requested a review from rosecers November 24, 2020 09:36
@rosecers

Copy link
Copy Markdown
Contributor

Please double check that the notebook runs clean and then feel free to merge.

@rosecers rosecers merged commit 54662e5 into lab-cosmo:master Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants