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

Add an example using custom distance for nearest search #1115

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

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Jun 23, 2024

No description provided.

@aprokop aprokop added the examples Anything to do with examples label Jun 23, 2024
@aprokop aprokop requested a review from dalg24 July 19, 2024 15:50
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the callback to reduce complexity and explore defining struct MyPoint { float x, y; };

examples/custom_distance/example_custom_distance.cpp Outdated Show resolved Hide resolved
examples/custom_distance/example_custom_distance.cpp Outdated Show resolved Hide resolved
examples/custom_distance/example_custom_distance.cpp Outdated Show resolved Hide resolved

// Expected output:
// offsets: 0 2
// distances: 0.5 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you elect to get distances out instead of indices?
I suspect indices would make it more obvious that the distance metric was changed.
I find the use of both a custom geometry with specialized distance computation mixed with a callback that recomputes the distance potentially confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose distances so that a user can see that the computed distances are indeed the custom ones. Computing just the found indices seems insufficient to me, as it still does not confirm that we can get the correct distance in the callback.

@aprokop
Copy link
Contributor Author

aprokop commented Jul 19, 2024

explore defining struct MyPoint { float x, y; };

The whole point is for a user to not define their own type. The only thing that's necessary is to have a separate type for disambiguation during the dispatch.

@aprokop
Copy link
Contributor Author

aprokop commented Jul 31, 2024

@dalg24 Do you have any blocking objections to merging the example in the current form?

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formally blocking. I find the complexity unnecessary (along the lines of my previous comments)

@aprokop
Copy link
Contributor Author

aprokop commented Jul 31, 2024

I chose distances so that a user can see that the computed distances are indeed the custom ones. Computing just the found indices seems insufficient to me, as it still does not confirm that we can get the correct distance in the callback.

@dalg24 If you find complexity unnecessary, please provide a reply to this comment.

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

Successfully merging this pull request may close these issues.

2 participants