-
Notifications
You must be signed in to change notification settings - Fork 52
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
Decouple kernel computation class initialisation from kernel #328
Conversation
def cross_covariance( | ||
self, x: Num[Array, "N D"], y: Num[Array, "M D"] | ||
self, kernel: Kernel, x: Num[Array, "N D"], y: Num[Array, "M D"] |
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.
Noticed some of the signatures for cross_covariance
and gram
use Num
instead of Float
. Not sure if these were intended (or how they crept in if not)? But the were here prior to your PR, so can be addressed in a separate issue.
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.
Yes I was also confused about that, and apparently it's because the GraphKernel
implementation accepts integer types. I had all Num
s changed to Float
but beartype was complaining so I had to switch back...for now 😅
|
||
Kernel = tp.TypeVar("Kernel", bound="gpjax.kernels.base.AbstractKernel") # noqa: F821 |
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.
Nice!
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.
Works with beartype but static type checkers complain about this way of annotating the bound, but all alternative solutions I tried resulted in circular imports errors at runtime type checking as explained in #293 (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.
Hi @frazane, nice PR, exactly what we wanted to see for #328.
Just had a single comment about a docstring / signature missing for a public method. And somethings up with the documentation test - will take a look at that shortly.
Aside, other-things, there are Num
instead of Float
's in places that can be addressed in a separate PR. And I like that the computation uses the first slot as the input for the kernel PyTree, it would be good to mimic this convention for the likelihood integrator's, and to add your nice TypeVar
annotation for the Likelihood in integrators.py too. Will open an issues for these.
LGTM. Happy for you to merge @frazane |
Type of changes
Checklist
poetry run pre-commit run --all-files --show-diff-on-failure
before committing.Description
Closes #293