Added BVH accelerated point localization and updated localization API - #345
Added BVH accelerated point localization and updated localization API#345HelenW42 wants to merge 19 commits into
Conversation
…on for Mapping2D and -3D
jacobmerson
left a comment
jacobmerson
left a comment
There was a problem hiding this comment.
A few initial things to work on. I will return to this in a little while with some additional feedback.
|
|
||
| #include <any> | ||
|
|
||
| #include <ArborX.hpp> |
There was a problem hiding this comment.
We need to make sure that we can build without ArborX. This will require adding a configure-time option PCMS_ENABLE_ARBORX to the CMakeLists.txt as well as an entry in configuration.h.in.
| #include <ArborX.hpp> | ||
| #include <ArborX_Triangle.hpp> | ||
| #include <detail/ArborX_PairValueIndex.hpp> | ||
| #include <detail/ArborX_AttachIndices.hpp> |
There was a problem hiding this comment.
We need to carefully review if APIs from detail are needed. Typically things in detail files or namespaces are internal implementation details and liable to change without notice.
| #include <Omega_h_mesh.hpp> | ||
| #include <Omega_h_bbox.hpp> | ||
| #include <Omega_h_shape.hpp> | ||
| #include <Omega_h_matrix.hpp> | ||
| #include <Omega_h_simplex.hpp> |
There was a problem hiding this comment.
Make sure we only include these if PCMS is built with Omega_h. I.e., use #IFDEF PCMS_ENABLE_OMEGA_H
| namespace detail | ||
| { | ||
|
|
||
| /** |
There was a problem hiding this comment.
Is this needed in the header, or can it be moved to the .cpp file?
| * compatability with ArborX::BVH | ||
| */ | ||
| template <int dim> | ||
| struct Omega_h_Mesh_Adapt |
There was a problem hiding this comment.
Can this be moved to the CPP file? It seems like an implementation detail we don't want to expose in the header.
There was a problem hiding this comment.
- Rather than commenting on every instance I think most of what's here can be moved to the
.cppfile since it is specific template instantiations that are only used to drive the implementation of the localization class. - We are slowly moving all header files over to
.hppsince this is a new file, let's use the.hppextension.
There was a problem hiding this comment.
I'm a bit confused about the current file naming (I understand that much of this was inherited). I think we should roughly have the following:
localization.hpp: include base class pure virtual interface for localizationuniform_grid_localization.hpp: class definition for uniform grid-based localization inheriting from the base class.arbor_x_localization.hpp: class definition for ArborX- associated .cpp files
We should ameks sure that as much code as possible is in the .cpp file rather than the header file. This a) makes it easier for the consumer to know how to interact with the library, b) limits the likelyhood of conflicts from consumers c) increases the compile performance
| Kokkos::parallel_for("copy_coords", n_pts, copy_functor); | ||
|
|
||
| auto results_d = search(coords_d); | ||
| auto results_d = search.apply(coords); |
There was a problem hiding this comment.
Make sure any methods have PascalCase (capitalize the first letter of every word).
| mesh_localization.cpp | ||
| point_localization.cpp | ||
| ) | ||
| find_package(ArborX REQUIRED) |
There was a problem hiding this comment.
Only require this if PCMS_ENABLE_ARBORX=ON
2 participants
This PR uses
ArborX::BVHto provide accelerated point localization and refactors the previous localization API (pcms::PointLocalizationSearch) to no longer use templates.