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.

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

ArborX neighbor search #250

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

ArborX neighbor search #250

wants to merge 40 commits into from

Conversation

kuberry
Copy link
Collaborator

@kuberry kuberry commented Oct 1, 2021

Add ArborX as an option for neighbor search, enabling GPU accelerated neighbor search.

What has been done:

  • Get radius search working (CPU/GPU)
  • Get KNN search working (CPU/GPU)
  • Add ArborX source files
  • Refactor tests to work with floating point neighbor search

What remains:

  • Move ArborX source files into tpls folder
  • Make neighbor searches do parallel work in execution space corresponding to memory space of vectors provided (if any have a device memory space, use this)
  • Switch search interface to pass by reference rather than pass by value, enabling resizing of neighbor lists in the search call. Replace the dry run / real search process with a single call that does both.
  • Replace CPU sort routine in ArborX with updated version

This PR changes the API for neighbor search with respect to the location of is_dry_run becoming do_dry_run, getting a default value of true (never to be changed manually by a user).

- Option check_same=true by default now, as the dry-run,
  then allocating, then re-running approach checks if the same
  number of neighbors are found for each target, in each search.

- For searches without a dry-run, this check would previously
  always fail, so this allows a disable of that check.
- If Compadre_USE_ARBORX is not set to on, then default back to
  nanoflann search routine
…for floats

- Added SEARCH_SCALAR_EPS conditional on which neighbor search routine
  is being used (1e-7 for ArborX and 1e-14 for Nanoflann) because
  ArborX searches using floats and Nanoflann searches using doubles.

- Modified tests and search routine to make the various casts explicit.
…changes API)

- ArborX neighbor search will ignore all do_dry_run bool flags
  and do full neighbor search with no recursive calls
- Nanoflann is broken until next commit when dry-run/real-run is
  built into recursive calls
- Switches itself from dry-run to storing results (with resizing)
  all through a single call
- Changed size_t returns from search methods to void
- Replaces internals of 2D searches for Nanoflann
- Replaces two definitions with one for both radius and KNN search
- Removed parameter do_dry_run from all rank 2 layout searches
@kuberry kuberry changed the title Arborx neighbor search ArborX neighbor search Oct 2, 2021
@kuberry kuberry added the TOOLKIT label Oct 6, 2021
- Changed a type to a decltype
@aprokop
Copy link

aprokop commented Oct 28, 2021

Overall, I think the code in Compadre_PointCloudSearch_ArborX.hpp looks fine. I think you do some unnecessary deep copies, though. For example, in both generateCRNeighborListsFromRadiusSearch and generateCRNeighborListsFromKNNSearch, you don't really need d_neighbor_lists. In KNN, you don't need to do Kokkos::deep_copy(device_execution_space(), d_neighbor_lists, neighbor_lists); in the beginning, as it's going to be overwritten anyway.

But this are small potatoes.

@aprokop
Copy link

aprokop commented Nov 2, 2021

@kuberry Just a quick question: do you plan to use ArborX path for any real applications? Or is the plan right now to simply have it in if a need arises? I would be really interested to see the benefits if its the former, and see what can be improved.

@kuberry
Copy link
Collaborator Author

kuberry commented Nov 3, 2021

@kuberry Just a quick question: do you plan to use ArborX path for any real applications? Or is the plan right now to simply have it in if a need arises? I would be really interested to see the benefits if its the former, and see what can be improved.

Compadre is a Trilinos package as well, so it is hard to say who all of the external users are. It is used in a number of projects at Sandia. Many of them that relate to circuit simulation are calling it in serial, however (query a single current for a voltage, query a single current for a voltage, ....).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants