HammingGraph space#170
Conversation
…ex, fix some phrasing/references.
…oper exceptions. Remove parameter in
vabor112
left a comment
vabor112
left a comment
There was a problem hiding this comment.
Great job, thank you, Colin! I'm leaving a few comments to discuss. Additionally, a code update will follow shortly.
| """ | ||
| return count_nonzero(logical_xor(x1[:, None, :], x2[None, :, :]), axis=-1) | ||
| x1_int = B.cast(int_like(x1), x1) | ||
| x2_int = B.cast(int_like(x2), x2) |
There was a problem hiding this comment.
Why do you need this? Shouldn't we expect integers here?
There was a problem hiding this comment.
Also, perhaps x1: B.Bool, x2: B.Bool should be changed to x1: B.Int, x2: B.Int now that we admin non-binary categories.
There was a problem hiding this comment.
tests/feature_maps/test_feature_maps.py is going to call lines 114-116 in geometric_kernels/feature_maps/random_phase.py:
phi_product = self.eigenfunctions.phi_product(
X, random_phases_b, **params
) # [N, O, L]
Here, for the HammingGraph space, X will be an integer, but random_phases_b will be a float... leading to a mismatch later on in the hamming_distance function.
There was a problem hiding this comment.
Move the cast into a more relevant location pls
| atol=1e-10, | ||
| ) | ||
|
|
||
| if d > 5: |
There was a problem hiding this comment.
What's the motivation behind this?
There was a problem hiding this comment.
Yes, I agree: this d > 5 is a bit arbitrary... I decided to keep it for coherence with the test_hypercube_graph_heat_kernel function (same file), which has a similar d > 5 structure.
| $$ | ||
| where $\omega_q = e^{2\pi i/q}$ is a primitive $q$-th root of unity and $x = (x_0, \ldots, x_{d-1}) \in H(d,q)$. | ||
|
|
||
| The eigenfunctions with the same number of non-zero frequency components $j = |\{i : s_i \neq 0\}|$ share the same eigenvalue $\lambda_j = (q-1)j / d$ and form a *level* $j$. The dimension of level $j$ is $\binom{d}{j}(q-1)^j$. |
There was a problem hiding this comment.
Doesn't coincide with the value in get_eigenvalues in HammingGraph:
(self.n_cat * level)
/ (
self.dim * (self.n_cat - 1)
) # we assume normalized Laplacian (for numerical stability)
There was a problem hiding this comment.
My bad... I believe the fraction in the code is the right one, and the above text should be modified with:
$lambda_j = \frac{q j}{(q-1) d$
vabor112
left a comment
vabor112
left a comment
There was a problem hiding this comment.
Double checked the theory, LGTM! Thanks again for the PR.
2 participants
Two related changes:
HammingGraph), which generalize hypercube graphs (HypercubeGraph) from binary to categorical.HypercubeGraphandHammingGraph, although this new implementation is not always faster... seenotebooks/HammingGraph.ipynb.