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

Taking the square root of distances is now optional #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hochshi
Copy link

@hochshi hochshi commented Feb 20, 2023

I've added a root_dist flag that allow the distances returned from nanoflann to stay squared even when using a L2 norm.
I've also added 2 tests to verify the addition of the flag the results remain consistent with sklearn.

Also bumped the version in setup.py to 0.0.9.

@u1234x1234
Copy link
Owner

Hi,

Thank you for the contribution!

What do you think about making it as a different metric name not as an additional root_dist argument?
I mean that root_dist does not affect L1 distance and it's not clear what to expect in cases (metric='L1', root_dist=True) or (metric='L1', root_dist=False).

My suggestion is to add a new metric L2_squared in addition to L1 and L2. And when the metric is L2_squared keep the distances squared by not taking root. So it will be something similar to:

if metric == "l2":
  dists = np.sqrt(dists)    # will be skipped with metric="l2_squared"

or

    if metric == "l2":
        g_d = [np.sqrt(x) for x in g_d]

@hochshi
Copy link
Author

hochshi commented Feb 22, 2023

Sure. I'll make the changes soon.
I was surprised at first that the distances were square rooted, as nanoflann's behaviour is to return squared distances.

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

Successfully merging this pull request may close these issues.

2 participants