Refactor DistributedTree to support APIv2 #1017
Conversation
aprokop
added
API
| template <typename MemorySpace> | ||
| struct CallbackWithDistance<BoundingVolumeHierarchy< | ||
| MemorySpace, Details::LegacyDefaultTemplateValue, | ||
| Details::DefaultIndexableGetter, ExperimentalHyperGeometry::Box<3, float>>> | ||
| { |
There was a problem hiding this comment.
The really bad. Can't get rid of the old legacy thingie just yet, as we cannot get a bounding volume by index.
There was a problem hiding this comment.
Why do we need both the primary template and that specialization?
There was a problem hiding this comment.
The primary template is based on a general tree, which really means the APIv2 tree. We have access to indexable and values, so don't need to store reverse permutation.
The specialization is for the APIv1 (specified through the default template parameters, including LegacyDefaultTemplateValue). We don't have access to indexable there. Even if we did, the problem is that this callback is wrapped into LegacyCallbackWrapper inside APIv1, so we don't have access to the full PairValueIndex value and can't call indexable on it. So, we have to do the other things.
This is pretty much unavoidable, as we have to use APIv1 BottomTree for APIv1 DistributedTree to be able to work with APIv1 post-callback interface in the distributed setting.
aprokop
force-pushed
the
distributed_apiv2_attempt3
branch
from
6730eca to
2f0196b
Compare
| int _rank; | ||
|
|
||
| template <typename Predicate, typename OutputFunctor> | ||
| KOKKOS_FUNCTION void operator()(Predicate const &, int primitive_index, |
There was a problem hiding this comment.
Haven't looked through the rest but my initial reaction is "why is that parameter int and not PairValueIndex?"
There was a problem hiding this comment.
Because it's being used with BVH<MemorySpace> which will do the callback wrapping on its own.
src/ArborX_LinearBVH.hpp
Outdated
| @@ -45,6 +45,8 @@ struct PerThread | |||
| namespace Details | |||
| { | |||
| struct HappyTreeFriends; | |||
| template <typename Tree> | |||
| struct CallbackWithDistance; | |||
There was a problem hiding this comment.
This is absurd. Just add an accessor to indexable getter to BVH and BF.
Or add to HappyTreeFriend if you have a good reason to think we should not do so.
FWIW boost::geometry::index::rtree has a indexable_get() member function.
There was a problem hiding this comment.
I introduced the accessor function. Named it indexable_get() following Boost, but not really a fan of it. Not sure what is a better name. indexable_getter() or indexableGetter()?
| Kokkos::is_view<Ranks>{} && Kokkos::is_view<Distances>{}> | ||
| typename Ranks> | ||
| static std::enable_if_t<Kokkos::is_view<Indices>{} && | ||
| Kokkos::is_view<Offset>{} && Kokkos::is_view<Ranks>{}> |
|
@dalg24 Please take another look at this one. |
| template <typename MemorySpace> | ||
| struct CallbackWithDistance<BoundingVolumeHierarchy< | ||
| MemorySpace, Details::LegacyDefaultTemplateValue, | ||
| Details::DefaultIndexableGetter, ExperimentalHyperGeometry::Box<3, float>>> | ||
| { |
There was a problem hiding this comment.
Why do we need both the primary template and that specialization?
DistributedTreeBaseThis should also satisfy the requirements for #907, though the interface may be a bit hard to use initially.
I opted to go for the base templated on the bottom tree because all other methods were too intrusive between spatial implementation and wrapping the post callback.