-
-
Notifications
You must be signed in to change notification settings - Fork 59
Initial commit of pivoted Cholesky algorithm from GPyTorch #63
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
Conversation
import torch | ||
|
||
from gpytorch.utils.permutation import apply_permutation |
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 think It'd be best to make torch
and gpytorch
optional imports.
try:
import torch
import gpytorch
except ImportError as e:
raise ImportError("message")
That way they don't need to be dependencies for someone who wan'ts to use pymc_experimental
for something else.
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.
Agreed
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've looked into the apply_permutation function, I think it is better to avoid the dependency and implement the same function here
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.
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.
100% agree @ferrine, longer term that'll have to implemented here, but this PRs more at the proof-of-concept level so lot's of time to go back over those details later. With the try...except
it shouldn't get in anyones way.
Same comments as the other PR, ready to be taken off of draft? |
… installed, and don't want to use pivoted Cholesky, can use pymc
@@ -0,0 +1,24 @@ | |||
# try: |
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.
Why you comment the test?
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.
This was triggering the import of GPyTorch
and Torch
which would cause the tests to fail. I thought it would be best to comment out the tests until I remove the dependency of my code on GPyTorch and Torch.
import torch | ||
from gpytorch.utils.permutation import apply_permutation | ||
except ImportError as e: | ||
# print( |
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.
instead of a print use raise ImportError(...)
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.
definitely, also consider raise ImportError(...) from e
https://stackoverflow.com/questions/24752395/python-raise-from-usage
pp = lambda x: np.array2string(x, precision=4, floatmode="fixed") | ||
|
||
|
||
def pivoted_cholesky(mat: np.matrix, error_tol=1e-6, max_iter=np.Infinity): |
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.
me being annoying: np.inf
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.
Looks great @kunalghosh! Feel free to hit the green button whenever you're ready
import torch | ||
from gpytorch.utils.permutation import apply_permutation | ||
except ImportError as e: | ||
# print( |
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.
definitely, also consider raise ImportError(...) from e
https://stackoverflow.com/questions/24752395/python-raise-from-usage
Sorry, didn't notice the conflict on |
nice job @kunalghosh, sorry for the delay on my side. This is good progress. |
What is this PR about?
Fast exact Gaussian processes (Gardner et.al 2018) works on a
modified batch conjugate gradient descent
algorithm which needs a function to return the preconditioning matrix, given a kernel. This PR implements the function to compute the preconditioning matrix.This produces the same results as
gpytorch.pivoted_cholesky()
Test kernel matrices can be generated using the following code which generates random positive semi-definite matrices
Now passing the same kernel matrix to the two functions will yield the same output.