search

improbable-eng / phtree-cpp

PH-Tree C++ implementation by Improbable.
Apache License 2.0
22 stars 15 forks source link

distance: Add PhPointF to DistanceEuclidean #18

Closed ctbur closed 2 years ago

ctbur commented 2 years ago

Changes

Add a function to calculate the L2 distance between two PhPointFs to DistanceEuclidean.

Verification

It's already used in tests.

improbable-til commented 2 years ago

This looks like a sensible change, however it means breaking the API. I won't merge it now but we could leave it open until a next major release?

ctbur commented 2 years ago

I don't see how it would break the API since the only adds a function and modifies some code for testing. Is it because of something with C++ templates that I am not aware of? Either way it's not urgent, it can wait until the next major release.

improbable-til commented 2 years ago

It would break the API because it removes DistanceEuclideanFloat. Removing it would may break any code that uses it, such as begin_query_knn for float, same as the unit test you had to adapt.

ctbur commented 2 years ago

Ah, but DistanceEuclideanFloat is only declared within phtree_f_test.cc, so it should not be usable outside that compilation unit. Aside from modifying the test file the PR only adds one function in distance.h.

improbable-til commented 2 years ago

Sorry for the delay. You are right, I thought it is all in distance.h.