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

Remove APIv1 shortcuts #1182

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Remove APIv1 shortcuts #1182

merged 5 commits into from
Nov 11, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Oct 14, 2024

Get out of business trying to guess user intentions. Make a user explicitly state what their desire is.

  • Introduce Experimental::Iota
  • Get rid of the shortcuts in BVH and BruteForce that return View<int> for views based on PairValueIndex<Geometry,int>
  • Update examples and benchmarks with Iota

Using an Iota instead of PairValueIndex does have a performance penalty, up to 40% in some cases. On Frontier,

BM_construction<ArborX::BVH<HIP>>/1000/0/manual_time_mean                                                +0.0127         +0.0127           181           183           184           187
-BM_radius_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/0/2/manual_time_mean                                 +0.0849         +0.0821           541           587           559           604
BM_radius_callback_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/0/2/manual_time_mean                        +0.0922         +0.0853           243           266           267           290
BM_knn_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/2/manual_time_mean                                      +0.0344         +0.0339           754           780           771           797
-BM_knn_callback_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/2/manual_time_mean                             +0.0514         +0.0496           581           611           603           632
BM_construction<ArborX::BVH<HIP>>/1000/1/manual_time_mean                                                +0.0105         +0.0106           186           188           190           192
-BM_radius_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/1/3/manual_time_mean                                 +0.0739         +0.0704           380           408           398           426
-BM_radius_callback_search<ArborX::BVH<HIP>>/1000/1000/10/1/0/1/3/manual_time_mean                        +0.0770         +0.0688           168           181           191           205
BM_knn_search<ArborX::BVH<HIP>>/1000/1000/10/1/1/3/manual_time_mean                                      +0.0274         +0.0271           693           712           710           729
BM_knn_callback_search<ArborX::BVH<HIP>>/1000/1000/10/1/1/3/manual_time_mean                             +0.0480         +0.0462           519           544           540           565
-BM_construction<ArborX::BVH<HIP>>/10000/0/manual_time_mean                                               +0.0077         +0.0071           212           213           215           217
-BM_radius_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/0/2/manual_time_mean                               +0.0782         +0.0763           685           739           703           757
-BM_radius_callback_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/0/2/manual_time_mean                      +0.0847         +0.0803           319           346           342           370
BM_knn_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/2/manual_time_mean                                    +0.0287         +0.0286           941           968           958           986
-BM_knn_callback_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/2/manual_time_mean                           +0.0514         +0.0500           756           795           777           816
BM_construction<ArborX::BVH<HIP>>/10000/1/manual_time_mean                                               +0.0027         +0.0024           215           216           219           220
-BM_radius_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/1/3/manual_time_mean                               +0.0942         +0.0910           513           561           531           579
-BM_radius_callback_search<ArborX::BVH<HIP>>/10000/10000/10/1/0/1/3/manual_time_mean                      +0.1050         +0.0966           234           258           258           282
BM_knn_search<ArborX::BVH<HIP>>/10000/10000/10/1/1/3/manual_time_mean                                    +0.0331         +0.0327          1122          1159          1140          1177
BM_knn_callback_search<ArborX::BVH<HIP>>/10000/10000/10/1/1/3/manual_time_mean                           +0.0499         +0.0484           939           986           960          1007
BM_construction<ArborX::BVH<HIP>>/100000/0/manual_time_mean                                              +0.0004         +0.0007           442           442           467           467
-BM_radius_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/0/2/manual_time_mean                             +0.0772         +0.0615          1120          1206          1304          1384
-BM_radius_callback_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/0/2/manual_time_mean                    +0.0766         +0.0746           504           543           524           563
BM_knn_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/2/manual_time_mean                                  +0.0401         +0.0370          2792          2904          3062          3175
BM_knn_callback_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/2/manual_time_mean                         +0.0469         +0.0464          2661          2786          2691          2815
BM_construction<ArborX::BVH<HIP>>/100000/1/manual_time_mean                                              -0.0009         -0.0005           450           449           474           474
-BM_radius_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/1/3/manual_time_mean                             +0.1018         +0.1003           785           865           803           883
-BM_radius_callback_search<ArborX::BVH<HIP>>/100000/100000/10/1/0/1/3/manual_time_mean                    +0.1156         +0.1084           380           423           405           449
-BM_knn_search<ArborX::BVH<HIP>>/100000/100000/10/1/1/3/manual_time_mean                                  +0.0503         +0.0478          4296          4513          4565          4783
-BM_knn_callback_search<ArborX::BVH<HIP>>/100000/100000/10/1/1/3/manual_time_mean                         +0.0532         +0.0527          4087          4304          4116          4333
+BM_construction<ArborX::BVH<HIP>>/1000000/0/manual_time_mean                                             -0.0976         +0.1817          2150          1940          1600          1891
-BM_radius_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/0/2/manual_time_mean                           +0.2812         +0.2724          9291         11904          9597         12211
-BM_radius_callback_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/0/2/manual_time_mean                  +0.4242         +0.4145          4278          6093          4377          6191
BM_knn_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/2/manual_time_mean                                +0.0448         +0.0446         37059         38719         37324         38988
-BM_knn_callback_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/2/manual_time_mean                       +0.0607         +0.0605         30702         32567         30856         32722
+BM_construction<ArborX::BVH<HIP>>/1000000/1/manual_time_mean                                             -0.0889         +0.1855          2148          1957          1603          1900
-BM_radius_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/1/3/manual_time_mean                           +0.1798         +0.1671          3728          4398          4030          4704
-BM_radius_callback_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/0/1/3/manual_time_mean                  +0.2606         +0.2431          1756          2214          1937          2408
BM_knn_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/1/3/manual_time_mean                                +0.0296         +0.0291         59477         61238         59733         61469
BM_knn_callback_search<ArborX::BVH<HIP>>/1000000/1000000/10/1/1/3/manual_time_mean                       +0.0356         +0.0355         53149         55040         53305         55196

Ultimately, I think, we would need to document it properly and provide performance suggestions for a user.

@aprokop aprokop added refactoring Code reorganization API User visible interface modifications labels Oct 14, 2024
@aprokop aprokop force-pushed the apiv1_v2 branch 2 times, most recently from ce5eb55 to 6f50471 Compare October 15, 2024 13:56
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sold on Iota and I don't fully grasp why it is needed and how it fits in the big picture

src/spatial/detail/ArborX_Iota.hpp Outdated Show resolved Hide resolved
src/spatial/detail/ArborX_Iota.hpp Outdated Show resolved Hide resolved
@@ -178,6 +151,18 @@ KOKKOS_FUNCTION
typename Details::AccessValues<Values, PrimitivesTag>::memory_space,
typename Details::AccessValues<Values, PrimitivesTag>::value_type>;

template <typename ExecutionSpace, typename Values, typename IndexableGetter>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable if ExecutionSpace is indeed an execution space {?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. I think it is out of scope for this PR. Rather, we would need to do that for all our deduction guides, both BVH and BruteForce. Or just wait for concepts.

@aprokop
Copy link
Contributor Author

aprokop commented Nov 7, 2024

I am not sold on Iota and I don't fully grasp why it is needed and how it fits in the big picture

A common user pattern is that they want to get the indices of the positive search results, like we've been doing in our original version, and possibly execute a callback on those results. As such, the hierarchy's value type would need to be int (or similar). Such a hierarchy would need to provide a structure for Values in the constructor to generate int values. Providing Iota would allow users to not write their own (together with AccessTraits specialization).

@aprokop
Copy link
Contributor Author

aprokop commented Nov 9, 2024

I updated the PR to do only the part where we eliminate the APIv1 shortcuts but do no provide global Iota.

Comment on lines -77 to +106
ArborX::BoundingVolumeHierarchy bvh{
space, ArborX::Experimental::attach_indices(primitives)};
ArborX::BoundingVolumeHierarchy bvh{space, primitives, indexable_getter};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't see a good way to write this without Iota.

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor comments

benchmarks/bvh_driver/benchmark_registration.hpp Outdated Show resolved Hide resolved
benchmarks/bvh_driver/benchmark_registration.hpp Outdated Show resolved Hide resolved
benchmarks/bvh_driver/benchmark_registration.hpp Outdated Show resolved Hide resolved
@aprokop
Copy link
Contributor Author

aprokop commented Nov 11, 2024

Because Iota no longer is a general utility, I simplified its instances in the places where it is used.

@aprokop aprokop mentioned this pull request Nov 11, 2024
Copy link
Collaborator

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

@aprokop aprokop merged commit 8c047ee into arborx:master Nov 11, 2024
1 of 2 checks passed
@aprokop aprokop deleted the apiv1_v2 branch November 11, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API User visible interface modifications refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants