Graph kernel consistency with other kernels #560
Conversation
… kernel to allow SVGP shapes and dtypes
gpjax/kernels/non_euclidean/utils.py
Outdated
| return params[tuple_indices] | ||
|
|
||
|
|
||
| def calculate_S(kernel): |
There was a problem hiding this comment.
Can we rename this function to something more informative and add types please?
gpjax/kernels/non_euclidean/utils.py
Outdated
| return params[tuple_indices] | ||
|
|
||
|
|
||
| def calculate_S(kernel): |
There was a problem hiding this comment.
| def calculate_S(kernel): | |
| def calculate_heat_semigroup(kernel: GraphKernel) -> Float[Array, "N N"]: |
There was a problem hiding this comment.
I tried out having a GraphKernel type but that leads to circular import errors. A way to seperate out the calculate_heat_semigroup function in utils file would be to have a base Graph class and the GraphKernel inherit the base class. Then calculate_heat_semigroup could just check for the base type. I am not sure if that is necessary at this point or if many graph kernels actually use eigenvalues. Thus I put the calculate_heat_semigroup function in the graph file itself for now.
I checked the notebook as well and it seems all is fine.
There was a problem hiding this comment.
To bypass a circular import only for the needs of types, you can do
from __future__ import annotations
from import beartype.typing as tp
if tp.TYPE_CHECKING:
from gpjax.kernels.non_euclidean.graph import GraphKernel
Overall this looks good. Can you please bump the version in gpjax/__init__.py?
There was a problem hiding this comment.
You're a genius! Worked like a charm. Function is back in utils and bumped init
2 participants
Checklist
uv run poe formatbefore committing.Description
This is in reference to the issue #558 . I have created another helper function to calculate S and moved it out to utils instead. I have kept the eigen computation bits so that the tests don't fail.
Not sure if this is the preferred way. I did think of moving the S calculations to the
_call_function itself but it maybe required for other kernels in the future?Issue Number: #558