Access traits for predicates #117

dalg24 · 2019-10-01T16:02:50Z

No description provided.

…e predicates

dalg24 · 2019-10-02T13:49:11Z

This is ready for review.

@aprokop I compared on-node performance with 5589972 and there was no noticeable difference.
I ran 1M values and 100k queries.

dalg24 · 2019-10-02T14:14:40Z

I updated #107 as an evidence that this compile with non Kokkos view predicates

src/ArborX_LinearBVH.hpp

src/details/ArborX_DetailsBatchedQueries.hpp

sslattery · 2019-10-02T14:42:12Z

src/details/ArborX_DetailsBoundingVolumeHierarchyImpl.hpp


-  queries =
-      Details::BatchedQueries<DeviceType>::applyPermutation(permute, queries);
+  // FIXME  readability!  queries is a sorted copy of the predicates


What is the readability issue here?

Function argument is predicates and it is some unspecified type that is only required to have a valid matching specialization of the access traits and bam next line I create queries which is a Kokkos view and keep going as before.

src/details/ArborX_DetailsTags.hpp

src/details/ArborX_DetailsTreeConstruction.hpp

aprokop

Overall, it looks really good. There are two points I want to bring up:

The words predicates and primitives are really hard for me. It always takes me several seconds to remember what they are, so personally I would prefer something simpler. Am I the only one with this issue?
I'm not sure how Primitives DataAccess interacts with BoxTag and PointTag. Am I correct in understanding that the current approach does not allow for the fine search to be implemented at the same time?

aprokop · 2019-10-02T14:42:40Z

src/ArborX_LinearBVH.hpp

+        typename Details::TagHelper<Predicates, Traits::PredicatesTag>::type;
+    static_assert(std::is_same<Tag, Details::NearestPredicateTag>::value ||
+                      std::is_same<Tag, Details::SpatialPredicateTag>::value,
+                  "Invalid tag for the predicates");


Suggested change

"Invalid tag for the predicates");

"Invalid tag for the predicates. Tag must be either NearestPredicateTag or SpatialPredicateTag.");

Won't fix. Will be superseded by #113

aprokop · 2019-10-02T15:11:52Z

src/details/ArborX_DetailsTreeConstruction.hpp

+private:
+  using accessor_return_type =
+      std::decay_t<decltype(Traits::Access<T, TTag>::get(
+          std::declval<T const &>(), std::declval<int>()))>;


Why do you need the second declval?

Could probably replace with an integer but the second argument is needed because get takes two params.

masterleinad

Looks overall OK, but I have some questions below.

masterleinad · 2019-10-02T14:19:00Z

src/ArborX_LinearBVH.hpp

-    // FIXME lame placeholder for concept check
-    static_assert(Kokkos::is_view<Predicates>::value, "must pass a view");
-    using Tag = typename Predicates::value_type::Tag;
+    // FIXME placeholder for concept check


What else would you want to check here? Aren't the static_asserts below sufficient?

See #113
Would check that matching specialization exist, that it has the proper size() and get() static member functions, the memory_space member type, that the tag can be deduced for the dispatch, etc.
Without it the user gets error messages that are very hard to parse.

masterleinad · 2019-10-02T14:44:57Z

src/details/ArborX_DetailsTreeConstruction.hpp

@@ -36,6 +36,19 @@ namespace ArborX
 {
 namespace Details
 {
+
+template <typename T, typename TTag>


Please put some documentation here. Basically, TagHelper::type can be used for SFINAE ensuring that the Access specialization implements the required get interface (returning a registered Tag, without user speciallization Point or Box, or any type that has a Tag alias). TagHelper::type then contains a Tag for that return type.

No that's not what this is for. It is there to deduce the tag from the return type of the get() access traits.

... which is what I said in the last sentence.

masterleinad · 2019-10-02T15:14:02Z

src/details/ArborX_DetailsBoundingVolumeHierarchyImpl.hpp

-      Details::BatchedQueries<DeviceType>::applyPermutation(permute, queries);
+  // FIXME  readability!  queries is a sorted copy of the predicates
+  auto queries = Details::BatchedQueries<DeviceType>::applyPermutation(
+      permute, predicates);


So this doesn't impact performance?

This does not change the algorithm and the choices that were previously done, which was to make a sorted copy of the queries/predicates instead of accessing with indirection. I had also explored populating the results in permuted order and reverse the permutation at the end but that was slower.
We can try to not make a copy (i.e. drop queries) and do Access::get(predicates, permute(i)) but I don't want to explore this in this PR. Also I am pretty sure I had explored that option back then.

masterleinad · 2019-10-02T15:22:22Z

src/details/ArborX_DetailsTags.hpp

@@ -28,7 +28,7 @@ struct BoxTag
 {
 };

-template <typename Geometry>
+template <typename T, typename Enable = void>


Wouldn't it make sense to separate geometry tags and predicate tags? Is there any function that is supposed to handle both?

And put them what header instead? This is good enough.

masterleinad · 2019-10-02T15:22:27Z

src/details/ArborX_DetailsTags.hpp

@@ -45,6 +45,12 @@ struct Tag<Box>
  using type = BoxTag;
 };

+template <typename T>
+struct Tag<T>


Hmm... It looks a little bit weird to treat Point and Box separately here. I would be happier if Point and Box would also have a Tag alias and we would not have to specialize here.

Not going to happen here.

dalg24 · 2019-10-02T15:58:48Z

Overall, it looks really good. There are two points I want to bring up:

The words predicates and primitives are really hard for me. It always takes me several seconds to remember what they are, so personally I would prefer something simpler. Am I the only one with this issue?

I'm not sure how Primitives DataAccess interacts with BoxTag and PointTag. Am I correct in understanding that the current approach does not allow for the fine search to be implemented at the same time?

What other terminology would you like to suggest?
Not necessarily. Storing user geometries instead of boxes in the leaf nodes would require non trivial changes so the geometry tag needed at construction is not a big concern at the moment. I would approach the fine search problem by "piggy packing" some work when collision are found. My current thinking is I will solve it via some new access traits for the results, with a specialization for a new second template argument ResultsTag.

dalg24 added 13 commits October 1, 2019 21:45

Drop static assertion that Predicates is a Kokkos view

5c331d4

Drop static assertion in GeometryTagHelper

c08f88e

Use Tag traits instead of std::conditional_t

db1e2d5

Rename GeometryTagHelper::{tag -> type}

688ce42

Change GeometryTagHelper<Primitives> -> TagHelper<T, TTag>

8e38aaa

Move definition of TagHelper outside TreeConstruction struct

2d0adbb

Use TagHelper in BVH::query() to get the predicates tag

3d687cf

Add static assertion to check that predicates tag is valid

af01955

Template BoundingVolumeHierarchyImpl::queryDispatch() functions on th…

6372cb4

…e predicates

Use access traits in Details::BoundingVolumeHierarchyImpl

14f8065

Add comment to emphasize coexistence of predicates and queries view

3b8ebdc

Propagate changes to Details::BatchedQueries

cb1947e

Use auto return type for nearest() just to spite Andrey

087406e

dalg24 force-pushed the access_traits_for_predicates branch from 3f05776 to 087406e Compare October 2, 2019 01:46

dalg24 marked this pull request as ready for review October 2, 2019 01:47

sslattery approved these changes Oct 2, 2019

View reviewed changes

aprokop approved these changes Oct 2, 2019

View reviewed changes

masterleinad reviewed Oct 2, 2019

View reviewed changes

Comment on what TagHelper is used for

3374b51

masterleinad merged commit c510eac into arborx:master Oct 2, 2019

dalg24 deleted the access_traits_for_predicates branch October 2, 2019 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access traits for predicates #117

Access traits for predicates #117

dalg24 commented Oct 1, 2019 •

edited

Loading

dalg24 commented Oct 2, 2019 •

edited

Loading

dalg24 commented Oct 2, 2019

sslattery Oct 2, 2019

dalg24 Oct 2, 2019

aprokop left a comment

aprokop Oct 2, 2019

dalg24 Oct 2, 2019

aprokop Oct 2, 2019

dalg24 Oct 2, 2019

masterleinad left a comment

masterleinad Oct 2, 2019

dalg24 Oct 2, 2019

masterleinad Oct 2, 2019

dalg24 Oct 2, 2019

masterleinad Oct 2, 2019

masterleinad Oct 2, 2019

dalg24 Oct 2, 2019

masterleinad Oct 2, 2019

dalg24 Oct 2, 2019

masterleinad Oct 2, 2019

dalg24 Oct 2, 2019

dalg24 commented Oct 2, 2019

	"Invalid tag for the predicates");
	"Invalid tag for the predicates. Tag must be either NearestPredicateTag or SpatialPredicateTag.");

Access traits for predicates #117

Access traits for predicates #117

Conversation

dalg24 commented Oct 1, 2019 • edited Loading

dalg24 commented Oct 2, 2019 • edited Loading

dalg24 commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 commented Oct 2, 2019

dalg24 commented Oct 1, 2019 •

edited

Loading

dalg24 commented Oct 2, 2019 •

edited

Loading