Skip to content
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.

Sign up for GitHub

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

Jump to bottom

APIv2: change signature of the 3-argument callback #965

Merged
merged 2 commits into from
Nov 3, 2023
Merged

APIv2: change signature of the 3-argument callback #965

merged 2 commits into from
Nov 3, 2023

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Oct 31, 2023 •
edited
Loading

Change the signature of the callback:

-callback(Predicate const &predicate, int primitive_index, OutputFunctor &functor);
+callback(Predicate const &predicate, Value const &value, OutputFunctor &functor);

@aprokop aprokop added enhancement New feature or request refactoring Code reorganization labels Oct 31, 2023
@aprokop aprokop requested a review from dalg24 October 31, 2023 16:43
aprokop
aprokop commented Oct 31, 2023
View reviewed changes
src/ArborX_BruteForce.hpp Outdated
Comment on lines 139 to 169
template <typename ExecutionSpace, typename Predicates, typename Callback,
typename OutputView, typename OffsetView, typename... Args>
std::enable_if_t<!Kokkos::is_view_v<std::decay_t<Callback>>>
query(ExecutionSpace const &space, Predicates const &predicates,
Callback &&callback, OutputView &&out, OffsetView &&offset,
Args &&...args) const
{
if constexpr (!Details::is_tagged_post_callback<
std::decay_t<Callback>>::value)
{
Details::check_valid_callback<int>(callback, predicates, out);
base_type::query(
space, predicates,
Details::LegacyCallbackWrapper{std::forward<Callback>(callback)},
std::forward<OutputView>(out), std::forward<OffsetView>(offset),
std::forward<Args>(args)...);
}
else
{
KokkosExt::ScopedProfileRegion guard("ArborX::BruteForce::query_crs");

Kokkos::View<int *, MemorySpace> indices(
"ArborX::CrsGraphWrapper::query::indices", 0);
base_type::query(space, predicates, Details::DefaultLegacyCallback{},
indices, std::forward<OffsetView>(offset),
std::forward<Args>(args)...);
callback(predicates, std::forward<OffsetView>(offset), indices,
std::forward<OutputView>(out));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the calls for the legacy classes out of CrsGraphImpl. The reason being is that the call stack was too complicated to figure out from within CrsGraphImpl when to wrap and when not to. Moving out made things doable.

The downside is that it had to be repeated in both BruteForce and Testing::LegacyTree. I think it's ok.

@aprokop aprokop mentioned this pull request Oct 31, 2023
Open
aprokop
aprokop commented Nov 1, 2023
View reviewed changes
src/ArborX_LinearBVH.hpp
@@ -172,23 +170,52 @@ class BoundingVolumeHierarchy
Experimental::TraversalPolicy const &policy =
Experimental::TraversalPolicy()) const
{
Details::check_valid_callback<int>(callback, predicates);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by change

aprokop
aprokop commented Nov 1, 2023
View reviewed changes
src/ArborX_BruteForce.hpp
@@ -123,21 +121,51 @@ class BruteForce
void query(ExecutionSpace const &space, Predicates const &predicates,
Callback const &callback, Ignore = Ignore()) const
{
Details::check_valid_callback<int>(callback, predicates);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by change

dalg24
dalg24 reviewed Nov 1, 2023
View reviewed changes
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor comments

test/tstCompileOnlyCallbacks.cpp Outdated Show resolved Hide resolved
test/ArborXTest_LegacyTree.hpp
std::enable_if_t<!Kokkos::is_view_v<std::decay_t<Callback>>>
query(ExecutionSpace const &space, Predicates const &predicates,
Callback &&callback, OutputView &&out, OffsetView &&offset,
Args &&...args) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in that case the Args template parameter pack is really just an "optional tree traversal policy" right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

src/details/ArborX_DetailsLegacy.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp
}
else
{
KokkosExt::ScopedProfileRegion guard("ArborX::BVH::query_crs");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we changing the stack of regions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This one does not go through the base class, so we have to add it here to retain the original stack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuk. Not sure if that is worth a comment to keep them in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is important. We don't guarantee anything about profiling regions stacks. As long as we name them somewhat related, it should be fine.

src/ArborX_BruteForce.hpp
Callback &&callback, OutputView &&out, OffsetView &&offset,
Args &&...args) const
{
if constexpr (!Details::is_tagged_post_callback<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just observing that we have this pattern 3x (BF, BVH, and LegacyTree)
Not sure whether we should/could abstract it away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Not ideal.

dalg24
dalg24 reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be nicer to forward the callback

src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp
}
else
{
KokkosExt::ScopedProfileRegion guard("ArborX::BVH::query_crs");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuk. Not sure if that is worth a comment to keep them in sync.

@aprokop aprokop merged commit 3080e6b into arborx:master Nov 3, 2023
1 of 2 checks passed
@aprokop aprokop deleted the callbacks-3-arg branch November 3, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request refactoring Code reorganization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@aprokop @dalg24