Implement expand half to full (neighbor list) #809
Conversation
|
Retest this please |
| #define ARBORX_TEST_EXPAND_HALF_TO_FULL(exec_space, offsets_in, indices_in, \ | ||
| offsets_out, indices_out) \ | ||
| BOOST_TEST(Test::expand(exec_space, offsets_in, indices_in) == \ | ||
| make_compressed_storage(offsets_out, indices_out), \ | ||
| boost::test_tools::per_element()) |
There was a problem hiding this comment.
I would use _half and _full instead of _in and _out for clarity, and maybe add _ref to the full ones.
| ARBORX_TEST_EXPAND_HALF_TO_FULL( | ||
| exec_space, (std::vector<int>{0, 1}), (std::vector<int>{0}), | ||
| (std::vector<int>{0, 2}), (std::vector<int>{0, 0})); |
There was a problem hiding this comment.
I must be missing something here and in the rest of the tests.
It seems that you are testing the expand function on the input that would be invalid outputs for the half traversal. In this particular case, you have a single element (0, 0) in the half-list, and it produces two (0, 0) in the output. This does not seem right.
There was a problem hiding this comment.
I added comments explaining what each assertion is checking in pair format
| Kokkos::deep_copy(space, offsets, 0); | ||
| Kokkos::parallel_for( | ||
| "ArborX::Experimental::count", | ||
| Kokkos::RangePolicy<ExecutionSpace>(space, 0, n), KOKKOS_LAMBDA(int i) { | ||
| for (int j = offsets_orig(i); j < offsets_orig(i + 1); ++j) | ||
| { | ||
| int const k = indices_orig(j); | ||
| Kokkos::atomic_increment(&offsets(i)); | ||
| Kokkos::atomic_increment(&offsets(k)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I would alias offsets to counts here for clarity.
| { | ||
|
|
||
| template <class ExecutionSpace, class Offsets, class Indices> | ||
| void expandHalfToFull(ExecutionSpace const &space, Offsets &offsets, |
There was a problem hiding this comment.
Just a note to myself: may want to explore a version of the function where we construct an array of pairs (i, j) and (j, i), sort it, and then construct new indices and offsets.
dalg24
force-pushed
the
expand_half_to_full
branch
from
add2bfa
to
f77318f
Compare
aprokop
changed the title
Per #801 (comment)