Skip to content

Conversation

@ctbur
Copy link
Contributor

@ctbur ctbur commented Jan 8, 2022

Changes

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

Verification

It's already used in tests.

@improbable-til improbable-til self-assigned this Jan 10, 2022
@improbable-til
Copy link
Contributor

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
Copy link
Contributor Author

ctbur commented Jan 17, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

ctbur commented Jan 19, 2022

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
Copy link
Contributor

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

@improbable-til improbable-til merged commit fec4837 into improbable-eng:master Jan 24, 2022
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