Skip to content
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

TSSS missing a factor of 2 #233

Open
jlmelville opened this issue Dec 9, 2023 · 0 comments
Open

TSSS missing a factor of 2 #233

jlmelville opened this issue Dec 9, 2023 · 0 comments

Comments

@jlmelville
Copy link
Collaborator

This was reported to me at jlmelville/rnndescent#8 but would also affect pynndescent, which is unsurprising as I directly converted the equation from here into C++.

Equation 7 of the TS-SS equation in the original paper (PDF), written in terms of degrees, has a denominator of 720, which would convert to 4pi in radians (the pi would cancel). As can be seen at:

sector = ((np.sqrt(d_euc_squared) + magnitude_difference) ** 2) * theta
triangle = norm_x * norm_y * np.sin(theta) / 2.0
return triangle * sector

there's only a division of 2 that shows up in the calculation of the return value.

I would volunteer to write a PR and update any affected test, but I would like to to use that paper as a means of confirming my understanding of the equations, as there are some partial numerical results in it. Unfortunately, I quickly got fed up trying to back-convert the vectors in the figures to plausible 2D descriptors (e.g. how can A and B in figure 1a be vectors of length 6, with an angle of 30 degrees between them, but a Euclidean distance of 2). Obviously this doesn't affect the returned neighbors but it could have a (small) effect for anything that used the distances downstream, like UMAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant