Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 DistributedTree Nearest query with callback #737
base: master
Are you sure you want to change the base?
Add DistributedTree Nearest query with callback #737
Changes from 37 commits
8223d33
a6cf7e6
5cf9a5a
6ce6cd2
a552d22
8587a9d
3928d3f
a637f27
ff91ac3
6aee1b5
1e6ba19
68bc0d5
eda8f6b
8379b23
9857b23
484db22
526d581
c4ad321
c666cfb
82505da
86b426a
e0d0d79
0ac9cc4
38bf593
5a26209
980404e
44ee75d
b5c622d
1c4beaf
8d40931
b72b0c8
9402639
d26ce07
3f66b82
7ac6480
2d19d6e
e2a899e
74f4a30
7452785
85ccd16
e6f43d5
591b89e
0baa3fd
3ffb9a7
901cd91
0601e4c
ec544e0
071f409
0d11bf1
dac4018
775e55b
fc59eca
498d407
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to guard it with SYCL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like our saying "intersection". Commenting so we can get back to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is deprecated, need to use the one that returns pairs of (index, rank).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the
Impl
versionqueryDispatchImpl
is not deprecated (and I rather use this one internally).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong for a few reasons.
First, it assumes that callback will output a single element for each match. In practice, we don't know how many elements the callback will spit out per query. It could be 0, or it could be more than 1. So we need to do a two pass here, similar to the
CrsGraphWrapperImpl
. I'm not sure what is the easiest way to do it without massive code duplication.Second, it assumes inline callback and not a post one. We could try to support post callback, but its meaning would have to be changed as we cannot execute it on all of the results coming from different ranks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also just assume that the callback only returns a single element to get started, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could. The thing is, this condition is undetectable, so the failure for a user would likely be an undefined behavior or a segfault. Which in the distributed setting would be tricky to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified by using
UnmanagedView
from the distributor data pointers, and doing an immediate deep_copoy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that what I'm doing already?