Change signature of the nearest callback #366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit only addresses on-node part.
aprokop
force-pushed
the
nearest_callback
branch
from
bff82e2
to
1a43969
Compare
|
Decided to quickly redo this as this will heavily conflict with #425. |
aprokop
commented
Dec 14, 2020
There was a problem hiding this comment.
Overall, the patch is pretty straightforward. The only thing I'm not happy with is where the extra functions (getReversePermutation and getPrimitiveBoundingVolume) live. Any ideas welcome. This should be merged prior to #425.
Comment on lines
49
to
59
| Kokkos::View<unsigned int *, typename BVH::memory_space> rev_permute( | ||
| Kokkos::ViewAllocateWithoutInitializing( | ||
| "ArborX::DistributedTree::query::nearest::reverse_permutation"), | ||
| n); | ||
| Kokkos::parallel_for( | ||
| "ArborX::DistributedTree::query::nearest::compute_reverse_permutation", | ||
| Kokkos::RangePolicy<ExecutionSpace>(exec_space, 0, n), | ||
| KOKKOS_LAMBDA(int const i) { | ||
| rev_permute(leaf_nodes(i).getLeafPermutationIndex()) = i; | ||
| }); |
There was a problem hiding this comment.
This was actually a nice idea. Mostly, because it avoids all the semantics of when and how to store user primitives, or extra arrays. It completely bypasses this question, because it is possible to figure out reverse permutation from the leaf nodes, which are accessible during query.
aprokop
commented
Dec 14, 2020
| @@ -417,6 +449,9 @@ DistributedTreeImpl<DeviceType>::queryDispatchImpl( | |||
| // recompute everything instead of just searching for potential better | |||
| // neighbors and updating the list. | |||
|
|
|||
| CallbackWithDistance<BVH<typename DeviceType::memory_space>> | |||
There was a problem hiding this comment.
Not sure if I should try to fix it to something like std::decay_t<decltype(bottom_tree)>. It is still completely tied to BVH and won't work with anything else.
There was a problem hiding this comment.
Just add a comment before you merge
aprokop
force-pushed
the
nearest_callback
branch
from
1a43969
to
40096ed
Compare
aprokop
force-pushed
the
nearest_callback
branch
from
40096ed
to
d5a1d29
Compare
|
@masterleinad Thanks for the review. I'll fix |
dalg24
reviewed
Dec 16, 2020
| return make_compressed_storage( | ||
| Kokkos::create_mirror_view_and_copy(Kokkos::HostSpace{}, offsets), | ||
| Kokkos::create_mirror_view_and_copy(Kokkos::HostSpace{}, values)); | ||
| } |
There was a problem hiding this comment.
Did you mean to keep the distributed version?
ArborX/test/Search_UnitTestHelpers.hpp
Lines 131 to 135 in d5a1d29
There was a problem hiding this comment.
DistributedTree has a (deprecated) query dispatch that returns distances. If we want to maintain backwards compatibility, it has to be kept and tested.
Co-authored-by: Damien L-G <dalg24@gmail.com>
|
@masterleinad @dalg24 Should have addressed all your comments. |
A friend declaration inside a class counts as a declaration, so if we also declare that friend outside the class, that will trigger a redundant declaration warning from this check.
|
PGI seems to fail sporadically, same error as in #430. PGI was running on |
dalg24
reviewed
Dec 18, 2020
This reverts commit 38d4358. No longer necessary, as we no longer use friend functions.
|
I changed to using friend class declaration instead of two friend functions. I also reverted clang-tidy check. @dalg24 This should address all your comments. On a side note, I wasted a lot of time trying to figure out why it did not work. It was an easy thing of forgetting to include This is not the first time I got burned by forgetting to include that header. Does anyone know if we can have a check that if a file uses any macro with a name from the config file, the config file has to be included? |
Our |
No. It only tests whether the standalone header can be compiled. But the issue here is that it can be compiled even when you use macros from the config header without including it. The protected code is simply compiled away. |
|
If it includes a macro, it should be in |
I guess I got burned accidentally here, as this |
|
again HIP failure |
|
retest this please |
dalg24
previously requested changes
Dec 18, 2020
dalg24
dismissed
their stale review
Unblocking now that error message is better
|
Another randomly failing test: distributed tree on PGI. Can be merged. |
|
retest this please |
|
@dalg24 You dismissed your review, but did not approve. Should I understand it as you are going to do a final look? |
dalg24
approved these changes
Dec 19, 2020
| @@ -417,6 +449,9 @@ DistributedTreeImpl<DeviceType>::queryDispatchImpl( | |||
| // recompute everything instead of just searching for potential better | |||
| // neighbors and updating the list. | |||
|
|
|||
| CallbackWithDistance<BVH<typename DeviceType::memory_space>> | |||
There was a problem hiding this comment.
Just add a comment before you merge
|
@dalg24 thanks! |
Merged
|
I completely forgot to performance test this PR. And results look very fishy (I compared 6224c97 with 9c4ae2d):
SerialOpenMPCuda |
aprokop
added a commit
that referenced
this pull request
Dec 22, 2020
Fix performance regression introduced in #366
aprokop
added a commit
to aprokop/ArborX
that referenced
this pull request
Dec 29, 2023
It is only used in the distributed testing, and does not test anything useful. It prevents the refactoring of the DistributedTree as it reaches inside the implementation which would vary between the old and new interface. Should have been removed as part of arborx#366.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #355