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

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Jun 23, 2024

Add an example that does nearest search using custom points and metric.

Drive-by change: remove couple unnecessary headers

@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; };


// 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.

@aprokop aprokop requested a review from dalg24 February 4, 2025 03:49
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.

Please fix the header includes before you merge

Comment on lines 52 to 53
// Provide the required centroid function for the custom data type
KOKKOS_FUNCTION auto returnCentroid(CustomPoint const &point) { return point; }
Copy link
Contributor

Choose a reason for hiding this comment

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

How comes we don't default that implementation?
(not blocking but we should consider fixing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do. I removed it. It was from the time when CustomPoint did not have operator[], and returnCentroid returned ArborX::Point and not CustomPoint.

@aprokop aprokop merged commit 25c7848 into arborx:master Feb 4, 2025
2 checks passed
@aprokop aprokop deleted the custom_distance branch February 4, 2025 13:55
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