-
Notifications
You must be signed in to change notification settings - Fork 41
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
Allow non device type template parameter for output views in query() on distributed trees #349
Allow non device type template parameter for output views in query() on distributed trees #349
Conversation
… distributed tree (spatial predicate only)
… distributed tree (nearest predicate)
…earchTreeImpl member functions
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.
Looks reasonable at a glance.
Co-authored-by: Andrey Prokopenko <andrey.prok@gmail.com>
typename Ranks> | ||
static std::enable_if_t<Kokkos::is_view<Indices>{} && | ||
Kokkos::is_view<Offset>{} && Kokkos::is_view<Ranks>{}> | ||
queryDispatch(SpatialPredicateTag, DistributedTree const &tree, |
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 you want to allow any other DistributedTree
in this pull request?
Otherwise, I would rather use
queryDispatch(SpatialPredicateTag, DistributedTree const &tree, | |
queryDispatch(SpatialPredicateTag, DistributedSearchTree<MemorySpace, void> const &tree, |
(here and below).
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 wanted to get rid of the forward declaration. Also this is one less place to update if we add template parameters to the distributed tree.
ExecutionSpace const &space, Predicates const &queries, | ||
Kokkos::View<int *, DeviceType> &indices, | ||
Kokkos::View<int *, DeviceType> &offset, | ||
Kokkos::View<int *, DeviceType> &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 should probably also check that these views have the correct rank (here and below).
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.
Yes
Basically #271 but for distributed trees
Cleaning up distributed tree implementation details is more involved than anticipated.
My short term plan is to get all
queryDispatch
overloads out of the struct templated on the device type andthen to introduce the new overloads that take a view of "pair" local index + rank instead of two separate views.