Finalize BasicBoundingVolumeHierarchy #915

aprokop · 2023-07-24T19:59:50Z

I added a Wiki to document the current state (though, Wiki contains RangeTraits which are not part of this PR).

Generally, I think BasicBoundingVolumeHierarchy is close to done. The things that remain:

~~Use RangeAdaptor instead of AccessTraits~~
Figure out if we want an additional template parameter, Parameters, similar to boost:

template<typename Value,
         typename Parameters,
         typename IndexableGetter = index::indexable<Value>,
         typename EqualTo = index::equal_to<Value>,
         typename Allocator = boost::container::new_allocator<Value>>

Worth thinking if BVH<MemorySpace, Value, IndexableGetter, Parameters...> is what we want, where Parameters... could include bounding volume and other parameters governing tree structure in the future.

We may also want to consider providing some convenient way to build a hierarchy based on pairs index + geometry, as that is a common use case. Maybe, rename LegacyValues to something more palatable to a user.

test/CMakeLists.txt

aprokop · 2023-07-25T21:14:12Z

Added wiki (see opening post).

masterleinad · 2023-07-26T15:27:34Z

Added wiki (see opening post).

Any reason not to create a pull request for the Wiki at this point?

aprokop · 2023-07-26T15:52:25Z

Any reason not to create a pull request for the Wiki at this point?

Created. I found it inconvenient to browse a PR, but now anyone can edit.

src/ArborX_LinearBVH.hpp

src/details/ArborX_AccessTraits.hpp

test/CMakeLists.txt

aprokop · 2023-08-30T16:47:02Z

OK, I removed RangeTraits changes, that was too much. Now it only changes the constructor.

aprokop · 2023-08-31T18:18:26Z

Huge performance regressions, investigating.

src/details/ArborX_IndexableGetter.hpp

aprokop · 2023-09-21T00:50:30Z

Summit

MST HACC 37M

master (2d3c687)

ArborX version    : 1.5 (dev)
ArborX hash       : 2d3c6877
Kokkos version    : 4.0.99
algorithm         : mst
minpts            : 1
filename          : /ccs/home/aprokop/csc333/hacc_37M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/hacc_37M.arborx" in binary mode...done
Read in 36902719 3D points
-- construction     :      0.103
-- boruvka          :      0.523
total time          :      0.645

branch (e557644)

ArborX version    : 1.5 (dev)
ArborX hash       : e557644a
Kokkos version    : 4.0.99
algorithm         : mst
minpts            : 1
filename          : /ccs/home/aprokop/csc333/hacc_37M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/hacc_37M.arborx" in binary mode...done
Read in 36902719 3D points
-- construction     :      0.090
-- boruvka          :      0.491
total time          :      0.599

MST 5D VisualSim 10M

master (2d3c687)

ArborX hash       : 2d3c6877                                                                                                                                   [7/48167]
Kokkos version    : 4.0.99
algorithm         : mst
minpts            : 1
filename          : /ccs/home/aprokop/csc333/5D_VisualSim_10M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/5D_VisualSim_10M.arborx" in binary mode...done
Read in 10000000 5D points
-- construction     :      0.039
-- boruvka          :      0.381
total time          :      0.427

branch (e557644)

ArborX version    : 1.5 (dev)
ArborX hash       : e557644a
Kokkos version    : 4.0.99
algorithm         : mst
minpts            : 1
filename          : /ccs/home/aprokop/csc333/5D_VisualSim_10M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/5D_VisualSim_10M.arborx" in binary mode...done
Read in 10000000 5D points
-- construction     :      0.034
-- boruvka          :      0.397
total time          :      0.437

DBSCAN HACC 37M

master (2d3c687)

ArborX version    : 1.5 (dev)
ArborX hash       : 2d3c6877
Kokkos version    : 4.0.99
algorithm         : dbscan
eps               : 0.042000
cluster min size  : 1
implementation    : fdbscan
verify            : false
minpts            : 5
filename          : /ccs/home/aprokop/csc333/hacc_37M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/hacc_37M.arborx" in binary mode...done
Read in 36902719 3D points
-- construction     :      0.103
-- query+cluster    :      0.336
---- neigh          :      0.060
---- query          :      0.262
-- postprocess      :      0.018
total time          :      0.461

#clusters       : 363244
#cluster points : 16958096 [45.95%]
#noise   points : 19944623 [54.05%]

branch (e557644)

ArborX version    : 1.5 (dev)
ArborX hash       : e557644a
Kokkos version    : 4.0.99
algorithm         : dbscan
eps               : 0.042000
cluster min size  : 1
implementation    : fdbscan
verify            : false
minpts            : 5
filename          : /ccs/home/aprokop/csc333/hacc_37M.arborx [binary, max_pts = -1]
samples           : -1
verbose           : true
Reading in "/ccs/home/aprokop/csc333/hacc_37M.arborx" in binary mode...done
Read in 36902719 3D points
-- construction     :      0.092
-- query+cluster    :      0.290
---- neigh          :      0.057
---- query          :      0.221
-- postprocess      :      0.018
total time          :      0.405

#clusters       : 363244
#cluster points : 16958096 [45.95%]
#noise   points : 19944623 [54.05%]

aprokop · 2023-09-21T03:28:38Z

@dalg24 OK, this should be ready. Please review.

masterleinad

I think the biggest question is how to expose this new interface to users who are using BVH directly. Should we create a new class and deprecate BVH or rather a backward incompatible change?

examples/triangle_intersection/triangle_intersection.cpp

src/ArborX_LinearBVH.hpp

src/details/ArborX_DetailsLegacy.hpp

test/tstKokkosToolsDistributedAnnotations.cpp

aprokop · 2023-10-05T22:33:11Z

I think the biggest question is how to expose this new interface to users who are using BVH directly. Should we create a new class and deprecate BVH or rather a backward incompatible change?

This is indeed the big question. I think in order to deprecate the old class, we would need to provide something new in the ArborX:: namespace. I think maybe rename BasicBoundingVolumeHierarchy to something more meaningful.

examples/triangle_intersection/triangle_intersection.cpp

src/details/ArborX_IndexableGetter.hpp

src/details/ArborX_DetailsLegacy.hpp

test/tstQueryTreeDegenerate.cpp

test/tstKokkosToolsDistributedAnnotations.cpp

test/tstQueryTreeCallbacks.cpp

test/tstDetailsTreeConstruction.cpp

aprokop · 2023-10-18T15:29:13Z

@masterleinad @dalg24 Addressed all your comments.

masterleinad

Fine with me.

src/ArborX_LinearBVH.hpp

test/CMakeLists.txt

aprokop · 2023-10-24T18:06:16Z

@dalg24 Done.

If both DefaultIndexableGetter has const ref pair argument, and Indexables have decltype(auto) return, this ends up in a wrong result (likely to the handling of temporaries). This leads to a default-initialized scene bounding box, which leads to same Morton codes, which leads to f-d up hierarchy. Providing a second operator()(PairIndexVolume<Geometry> &&pair) operator in DefaultIndexableGetter fixes that.

dalg24

Minor comments. Pretty good overall.

test/tstQueryTreeDegenerate.cpp

test/Search_UnitTestHelpers.hpp

test/CMakeLists.txt

dalg24 · 2023-10-25T22:15:53Z

test/CMakeLists.txt

      "using KDOP6 = ArborX::Experimental::KDOP<6>;\n"
      "using KDOP14 = ArborX::Experimental::KDOP<14>;\n"
      "using KDOP18 = ArborX::Experimental::KDOP<18>;\n"
      "using KDOP26 = ArborX::Experimental::KDOP<26>;\n"
-      "template <class MemorySpace> using ArborX__BoundingVolumeHierarchy_${_bounding_volume} = ArborX::BasicBoundingVolumeHierarchy<MemorySpace, ArborX::Details::PairIndexVolume<${_bounding_volume}>, ArborX::Details::DefaultIndexableGetter, ${_bounding_volume}>;\n"
+      "template <class MemorySpace>\n"
+      "using ArborX__BoundingVolumeHierarchy_${_bounding_volume} =\n"


We only want to test the legacy code path here?

Yes. BVH only has MemorySpace, no bounding volume, so impossible to run KDOPs with.

aprokop · 2023-10-26T13:31:16Z

The triangle example with mapping is unstable on SYCL (build, rebuild). Has nothing to do with the PR. Everything else is good.

dalg24 · 2023-10-26T15:12:03Z

test/CMakeLists.txt

-    "#define ARBORX_TEST_TREE_TYPES Tuple<ArborX::BVH>\n"
+    "#include \"ArborXTest_LegacyTree.hpp\"\n"
+    "template <class MemorySpace>\n"
+    "using ArborX_Legacy_BasicBVH_Box =\n"


A bit long mangled name would have done ArborX_LegacyBVH but not blocking

dalg24 · 2023-10-26T15:13:18Z

test/CMakeLists.txt

-      "template <class MemorySpace> using ArborX__BoundingVolumeHierarchy_${_bounding_volume} = ArborX::BasicBoundingVolumeHierarchy<MemorySpace, ArborX::Details::PairIndexVolume<${_bounding_volume}>, ArborX::Details::DefaultIndexableGetter, ${_bounding_volume}>;\n"
-      "#define ARBORX_TEST_TREE_TYPES Tuple<ArborX__BoundingVolumeHierarchy_${_bounding_volume}>\n"
+      "template <class MemorySpace>\n"
+      "using ArborX_Legacy_BasicBVH_${_bounding_volume} =\n"


Would drop the "basic" and say ArborX_LegacyBVH_. also not blocking

aprokop commented Jul 25, 2023

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

aprokop added enhancement New feature or request refactoring Code reorganization labels Jul 25, 2023

aprokop mentioned this pull request Jul 26, 2023

API v2 tracker #872

Open

aprokop mentioned this pull request Aug 14, 2023

Add example for 2D triangle-point intersection #542

Merged

aprokop mentioned this pull request Aug 24, 2023

Update triangle example to API v2 #936

Merged

aprokop force-pushed the 9-constructor_values branch from be54657 to 732dfd0 Compare August 30, 2023 16:15

aprokop marked this pull request as ready for review August 30, 2023 16:15

aprokop commented Aug 30, 2023

View reviewed changes

src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved

src/details/ArborX_AccessTraits.hpp Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

aprokop force-pushed the 9-constructor_values branch from 732dfd0 to f631ac2 Compare August 31, 2023 17:10

aprokop commented Sep 1, 2023

View reviewed changes

src/details/ArborX_IndexableGetter.hpp Outdated Show resolved Hide resolved

aprokop force-pushed the 9-constructor_values branch from 23e46e1 to 88e7990 Compare September 15, 2023 15:47

masterleinad reviewed Oct 5, 2023

View reviewed changes

dalg24 reviewed Oct 16, 2023

View reviewed changes

masterleinad approved these changes Oct 18, 2023

View reviewed changes

masterleinad mentioned this pull request Oct 18, 2023

Moving least squares interpolation #946

Merged

aprokop mentioned this pull request Oct 23, 2023

Implement APIv2 BruteForce #962

Merged

aprokop commented Oct 23, 2023

View reviewed changes

src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Oct 24, 2023

View reviewed changes

src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

aprokop force-pushed the 9-constructor_values branch from 34d74fc to 9eebbd9 Compare October 24, 2023 18:16

aprokop added 2 commits October 25, 2023 10:03

Change BasicBVH constructor

2338e1e

aprokop force-pushed the 9-constructor_values branch from 9eebbd9 to 0fdfa87 Compare October 25, 2023 14:05

dalg24 reviewed Oct 25, 2023

View reviewed changes

Minor changes to test/ to address comments

47022c7

dalg24 approved these changes Oct 26, 2023

View reviewed changes

aprokop merged commit 2abcb6c into arborx:master Oct 26, 2023
1 check passed

aprokop deleted the 9-constructor_values branch October 26, 2023 15:14

aprokop mentioned this pull request Nov 27, 2023

Consider increasing the minimum nvcc version from 11.0.3 to 11.4+ #974

Closed

aprokop mentioned this pull request Mar 15, 2024

Fix a temporary objects issue with DefaultIndexableGetter #1045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize BasicBoundingVolumeHierarchy #915

Finalize BasicBoundingVolumeHierarchy #915

aprokop commented Jul 24, 2023 •

edited

Loading

aprokop commented Jul 25, 2023

masterleinad commented Jul 26, 2023

aprokop commented Jul 26, 2023

aprokop commented Aug 30, 2023

aprokop commented Aug 31, 2023

aprokop commented Sep 21, 2023

aprokop commented Sep 21, 2023 •

edited

Loading

masterleinad left a comment

aprokop commented Oct 5, 2023

aprokop commented Oct 18, 2023

masterleinad left a comment

aprokop commented Oct 24, 2023

dalg24 left a comment

dalg24 Oct 25, 2023

aprokop Oct 26, 2023

aprokop commented Oct 26, 2023 •

edited

Loading

dalg24 Oct 26, 2023

dalg24 Oct 26, 2023

Finalize BasicBoundingVolumeHierarchy #915

Finalize BasicBoundingVolumeHierarchy #915

Conversation

aprokop commented Jul 24, 2023 • edited Loading

aprokop commented Jul 25, 2023

masterleinad commented Jul 26, 2023

aprokop commented Jul 26, 2023

aprokop commented Aug 30, 2023

aprokop commented Aug 31, 2023

aprokop commented Sep 21, 2023

Summit

MST HACC 37M

MST 5D VisualSim 10M

DBSCAN HACC 37M

aprokop commented Sep 21, 2023 • edited Loading

masterleinad left a comment

Choose a reason for hiding this comment

aprokop commented Oct 5, 2023

aprokop commented Oct 18, 2023

masterleinad left a comment

Choose a reason for hiding this comment

aprokop commented Oct 24, 2023

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 Oct 25, 2023

Choose a reason for hiding this comment

aprokop Oct 26, 2023

Choose a reason for hiding this comment

aprokop commented Oct 26, 2023 • edited Loading

dalg24 Oct 26, 2023

Choose a reason for hiding this comment

dalg24 Oct 26, 2023

Choose a reason for hiding this comment

aprokop commented Jul 24, 2023 •

edited

Loading

aprokop commented Sep 21, 2023 •

edited

Loading

aprokop commented Oct 26, 2023 •

edited

Loading