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.
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.
Yes I was also confused about that, and apparently it's because the GraphKernel implementation accepts integer types. I had all Nums 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.
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.
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-failurebefore committing.Description
Closes #293