-
Notifications
You must be signed in to change notification settings - Fork 35
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
Externalize crs graph #425
Conversation
The performance benchmark results look OK
|
One thing that I think breaks the modularity is that buffer optimization have to know about the fact whether acceleration structure reorders queries inside or not. So, they are not completely independent. But it's still better, I think. |
Thanks for posting. This is expected, as I did not touch buffer optimization code, except for making it run |
The updated version got rid of |
I think we need to understand of semantics of interaction of two-pass with the underlying data structure, and where preparation should happen (like sorting). But we can also kick the ball down the road, and postpone hard thinking. |
Rebased on master (fixed conflicts with #427). |
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 pretty good to me.
Is the code in details/ArborX_DetailsBufferOptimization.hpp
still used?
Reviewing would have been much easier by clearing marking commits that only move code around.
I don't have a strong opinion about BoostRtree and would be fine. Thinking about compatibility later.
That code was moved to
I understand that, but I could not find a simple way of just moving code around without breaks. Instead of spending a lot of time trying be very careful, I just ripped the bandage off. |
Yes, I realized that but you don't remove |
Huh... I guess I messed something up during the rebase, as this and |
This patch works for me: diff --git a/src/ArborX_CrsGraphWrapper.hpp b/src/ArborX_CrsGraphWrapper.hpp
index e91d70a..72695ad 100644
--- a/src/ArborX_CrsGraphWrapper.hpp
+++ b/src/ArborX_CrsGraphWrapper.hpp
@@ -24,9 +24,17 @@ inline void query_crs(ExecutionSpace const &space, Tree const &tree,
CallbackOrView &&callback_or_view, View &&view,
Args &&... args)
{
- Details::CrsGraphWrapperImpl::query(
- space, tree, predicates, std::forward<CallbackOrView>(callback_or_view),
- std::forward<View>(view), std::forward<Args>(args)...);
+ Details::CrsGraphWrapperImpl::
+ check_valid_callback_if_first_argument_is_not_a_view(callback_or_view,
+ predicates, view);
+
+ using Access = AccessTraits<Predicates, Traits::PredicatesTag>;
+ using Tag = typename Details::AccessTraitsHelper<Access>::tag;
+
+ ArborX::Details::CrsGraphWrapperImpl::queryDispatch(
+ Tag{}, tree, space, predicates,
+ std::forward<CallbackOrView>(callback_or_view), std::forward<View>(view),
+ std::forward<Args>(args)...);
}
} // namespace ArborX
diff --git a/src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp b/src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp
index fe51cf4..dbadd47 100644
--- a/src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp
+++ b/src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp
@@ -610,24 +610,6 @@ check_valid_callback_if_first_argument_is_not_a_view(View const &,
// do nothing
}
-template <typename ExecutionSpace, typename Tree, typename Predicates,
- typename CallbackOrView, typename View, typename... Args>
-inline std::enable_if_t<Kokkos::is_view<std::decay_t<View>>{}>
-query(ExecutionSpace const &space, Tree const &tree,
- Predicates const &predicates, CallbackOrView &&callback_or_view,
- View &&view, Args &&... args)
-{
- check_valid_callback_if_first_argument_is_not_a_view(callback_or_view,
- predicates, view);
-
- using Access = AccessTraits<Predicates, Traits::PredicatesTag>;
- using Tag = typename AccessTraitsHelper<Access>::tag;
-
- queryDispatch(Tag{}, tree, space, predicates,
- std::forward<CallbackOrView>(callback_or_view),
- std::forward<View>(view), std::forward<Args>(args)...);
-}
-
} // namespace CrsGraphWrapperImpl
} // namespace Details
diff --git a/test/tstDetailsBufferOptimization.cpp b/test/tstDetailsBufferOptimization.cpp
index 98878bf..af45977 100644
--- a/test/tstDetailsBufferOptimization.cpp
+++ b/test/tstDetailsBufferOptimization.cpp
@@ -27,13 +27,11 @@ struct Test1
typename InsertGenerator>
void query(ExecutionSpace const &space, Predicates const &predicates,
InsertGenerator const &insert_generator,
- ArborX::Experimental::TraversalPolicy const &policy =
+ ArborX::Experimental::TraversalPolicy const & =
ArborX::Experimental::TraversalPolicy()) const
{
using Access = ArborX::AccessTraits<Predicates, ArborX::PredicatesTag>;
- std::ignore = policy;
-
Kokkos::parallel_for(
Kokkos::RangePolicy<ExecutionSpace>(space, 0, Access::size(predicates)),
KOKKOS_LAMBDA(int predicate_index) {``` |
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
I'm OK applying it, but I want to see what @dalg24 says first. |
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 strongly object to the free function name query_crs
and suggest we call it just query
.
I am also tempted to always take the tree as first argument.
I'm OK with it. But lets say we want to introduce a function that stores the results in 2D format (like, CabanaMD). What do you propose to call that function? Or do you assume you can always figure out which one is called based on arguments? In addition, I find it a bit confusing with the proliferation of all
I have no strong arguments for either, and don't care. |
Decided not to waste time on this, and changed it as requested. |
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
I addressed all comments. The only remaining thing seems to be a discussion about the place of sorting in the algorithm. |
Merged |
I think there only two things remaining in this PR:
|
Fixed Boost thing. And I think it's ok to address the sorting issue in a separate PR. So, I think it is ready. Once I get the approvals, I will rerun the performance studies. |
Do not expose a `query` with an execution space in 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.
Should we hold on a bit longer before emitting deprecation warnings?
Comparison of master (6368455) and pr (cc01971) reveals no regression:
|
@dalg24 I addressed all your comments. This is ready for merge. |
Merged with SYCL test failed (see #442). |
Most stuff is just shuffling things around.
ArborX_DetailsCrsGraphWrapper.hpp
combines most ofArborX_BoundingVolumeHierarchy.hpp
withArborX_DetailsBufferOptimization.hpp
.