Cut down on the number of AccessTraits #967

aprokop · 2023-11-01T14:34:00Z

The goal is to avoid using AccessTraits internally in ArborX, only at the user interface functions. Motivated by #729, and in preparation for RangeTraits introduction.

aprokop · 2023-11-02T17:25:01Z

@dalg24 I checked performance using DBSCAN driver on A100 (both DBSCAN and MST), and have not observed any differences. Gitlab CI is clean too.

aprokop · 2023-11-03T17:16:21Z

Updated the PR to include AccessTraits<Predicates, PredicatesTag. Will check the performance again.

aprokop · 2023-11-04T18:49:44Z

CUDA 11.0.3 has something weird

ptxas fatal   : Unresolved extern function '_ZN6ArborX12AccessTraitsIN6Kokkos4ViewIPNS_10IntersectsINS_3BoxEEEJNS1_6DeviceINS1_4CudaENS1_9CudaSpaceEEEEEENS_13PredicatesTagEvE3getERKSB_i'

(aka ArborX::AccessTraits<Kokkos::View<ArborX::Intersects<ArborX::Box>*, Kokkos::Device<Kokkos::Cuda, Kokkos::CudaSpace> >, ArborX::PredicatesTag, void>::get(Kokkos::View<ArborX::Intersects<ArborX::Box>*, Kokkos::Device<Kokkos::Cuda, Kokkos::CudaSpace> > const&, int))

This looks like a compiler bug, but I don't know how to fix.

masterleinad · 2023-11-02T20:23:19Z

benchmarks/dbscan/ArborX_DBSCANVerification.hpp

+                                       ArborX::Details::PairIndexVolume<Point>>
+      bvh(exec_space, ArborX::Details::LegacyValues<Points, Point>{points});


This changes the leaf node type as well, right?

Yes. Instead of storing boxes, it will now store points. It was an oversight from some previous PRs.

masterleinad · 2023-11-06T13:27:54Z

src/ArborX_BruteForce.hpp

@@ -63,23 +63,23 @@ class BasicBruteForce
  void query(ExecutionSpace const &space, Predicates const &predicates,
             Callback const &callback, Ignore = Ignore()) const;

-  template <typename ExecutionSpace, typename Predicates,
+  template <typename ExecutionSpace, typename UserPredicates,


Why do you only change the name here and not in the other overloads? I would rather change the name in the implementation than in the interface. Maybe Predicates->PredicatesAccessor.

I changed the name only here because we will wrap the provided predicates in AccessValues. In other places, where no wrapping is done, it does not matter.

I don't feel strongly either way.

src/ArborX_DBSCAN.hpp

masterleinad · 2023-11-06T15:55:59Z

src/details/ArborX_AccessTraits.hpp

@@ -199,16 +199,18 @@ void check_valid_access_traits(PrimitivesTag, Primitives const &,
  }
 }

-template <typename Values>
+template <typename Values, typename Tag = PrimitivesTag>


If multiple tags can be used, I would rather not have a default value.

I thought about it. The thing is that in this case it would increase of size of this patch, as I would also have to change all places where I used AccessValues for primitives. I am not sure I want to do this.

As long as it's a separate commit, it's pretty clean/clear.

The goal is to avoid using AccessTraits internally in ArborX, only at the user interface functions. In preparation for RangeTraits introduction.

aprokop · 2023-12-07T00:39:56Z

DroppingAccessTraits<Predicates, PredicatesTag> from this PR. This should avoid CUDA-11.0.3 build issue, and unlock #969.

aprokop · 2023-12-07T02:16:29Z

Everything passes except HIP OOM.

dalg24 · 2023-11-02T03:23:26Z

src/ArborX_DBSCAN.hpp

@@ -263,8 +239,8 @@ dbscan(ExecutionSpace const &exec_space, Primitives const &primitives,
 {
  Kokkos::Profiling::pushRegion("ArborX::DBSCAN");

-  using Access = AccessTraits<Primitives, PrimitivesTag>;
-  using MemorySpace = typename Access::memory_space;
+  using Points = Details::AccessValues<Primitives>;


What is the reasoning for the parameters naming in DBSCAN?
It went for Primitives/Points as compared to the spatial indexes where we did UserValues/Values.

OK I see below that we assert the value type is point.

Generally, I try to name the variables based on the context. In DBSCAN, the values are points and nothing else. Spatial indexes have to deal with unknown values, and I thus call them UserValues. I use Primitives as something that has valid AccessTraits as a rule of thumb, to indicate that it uses the old interface.

src/details/ArborX_MinimumSpanningTree.hpp

aprokop added the refactoring Code reorganization label Nov 1, 2023

aprokop mentioned this pull request Nov 1, 2023

API v2 tracker #872

Open

aprokop force-pushed the access_traits_proliferation_primitives branch from 7c3118f to 15f9594 Compare November 1, 2023 14:40

aprokop changed the title ~~Cut down on the number of Access<Primitives, PrimitivesTag>~~ Cut down on the number of AccessTraits<Primitives, PrimitivesTag> Nov 1, 2023

aprokop force-pushed the access_traits_proliferation_primitives branch from 15f9594 to cd40a3d Compare November 3, 2023 17:15

aprokop changed the title ~~Cut down on the number of AccessTraits<Primitives, PrimitivesTag>~~ Cut down on the number of AccessTraits Nov 3, 2023

masterleinad reviewed Nov 6, 2023

View reviewed changes

aprokop mentioned this pull request Nov 27, 2023

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

Closed

Cut down on the number of AccessTraits<Primitives, PrimitivesTag>

e70bd45

The goal is to avoid using AccessTraits internally in ArborX, only at the user interface functions. In preparation for RangeTraits introduction.

aprokop force-pushed the access_traits_proliferation_primitives branch 2 times, most recently from aa590f6 to e70bd45 Compare December 7, 2023 00:39

aprokop requested review from masterleinad and dalg24 December 7, 2023 02:16

dalg24 approved these changes Dec 7, 2023

View reviewed changes

aprokop merged commit 5a8d82e into arborx:master Dec 7, 2023

aprokop deleted the access_traits_proliferation_primitives branch December 7, 2023 14:52

aprokop mentioned this pull request Dec 10, 2023

Cut down on the number of AccessTraits<Predicates, PredicatesTag> #978

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cut down on the number of AccessTraits #967

Cut down on the number of AccessTraits #967

aprokop commented Nov 1, 2023 •

edited

Loading

aprokop commented Nov 2, 2023 •

edited

Loading

aprokop commented Nov 3, 2023

aprokop commented Nov 4, 2023

masterleinad Nov 2, 2023

aprokop Nov 6, 2023

masterleinad Nov 6, 2023

aprokop Nov 6, 2023

masterleinad Nov 6, 2023

aprokop Nov 6, 2023

masterleinad Nov 6, 2023

aprokop commented Dec 7, 2023

aprokop commented Dec 7, 2023 •

edited

Loading

dalg24 Nov 2, 2023

dalg24 Nov 2, 2023

aprokop Dec 7, 2023

		ArborX::Details::PairIndexVolume<Point>>
		bvh(exec_space, ArborX::Details::LegacyValues<Points, Point>{points});

Cut down on the number of AccessTraits #967

Cut down on the number of AccessTraits #967

Conversation

aprokop commented Nov 1, 2023 • edited Loading

aprokop commented Nov 2, 2023 • edited Loading

aprokop commented Nov 3, 2023

aprokop commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop commented Dec 7, 2023

aprokop commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop commented Nov 1, 2023 •

edited

Loading

aprokop commented Nov 2, 2023 •

edited

Loading

aprokop commented Dec 7, 2023 •

edited

Loading