Add a very light non-performant version of APIv1 back - #1275
Conversation
|
Since we had to reimplement
Do we pay the penalty only on the leaf nodes? Is the penalty just that we need to access user data instead of access it directly inside ArborX? |
I'm not opposed to moving
Yes, only on the leaf nodes, internal nodes don't change. |
I meant moving Iota inside the library. It's fine in the current namespace.
I expect that the indirect reference would be small in most cases. If you are constructing object on the fly, it's fair to ask you to use a more advanced workflow. It would be nice to run a benchmark to compare the performance difference but otherwise I like it. |
I did do that at some point (see here). Would need to update and rerun that benchmark and post the numbers. |
|
Here are the results on Frontier ( The difference grows with size. When storing the neighbor indices, the slowdown on 100M is 43%. For pure callbacks, it's 28%. |
|
This is more than I expected but it's still worth to have something like this available for new users. We should make it clear that by switching from the simplified to the advanced API, people can expect at least 25% but sometimes even more throughput even for simple cases. This way people can start with the simplified API. If they need better performance, they use the advanced API and they can expect substantial improvement (not a 5% difference). |
I think it's really is case specific. It would depend on how many times each query hits a leaf, as well as the size of the problem. I can see it being anywhere from 5% to 50% improvement. |
|
My worry is that someone does a performance comparison between ArborX with the simple interface and another library. As long as we find a way to make that clear, I am fine with this interface. |
Yeah, that's my worry too. It's a tradeoff between making the first experience easy and making the first experience fast. Easy probably beats fast. |
|
The current situation is unacceptable. It's just too hard to try ArborX. We can add a note like Adios does |
|
@dalg24 What are your thoughts on this PR? Particularly, do you think it's a reach to provide second argument as |
|
TODO: make a new construct a free function (outside of indexes) |
There was a problem hiding this comment.
Pull request overview
This PR reintroduces a simpler, less-performant “APIv1-like” construction path by adding a ArborX::create_index(...) helper that builds BVH/BruteForce indices from a size and an indexable getter (internally using Details::Iota), then updates several call sites to use it.
Changes:
- Added
ArborX::create_indexoverloads (including MPI-enabled overloads) to construct indices from(ExecutionSpace, int size, indexable_getter). - Updated examples/tests/benchmarks to use
create_indexinstead of manualIotaplumbing /attach_indices. - Simplified one example query call to use the default callback path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/spatial/ArborX_CrsGraphWrapper.hpp |
Introduces create_index helper overloads (serial + MPI). |
test/tstQueryTreeIntersectsKDOP.cpp |
Switches BVH construction to create_index. |
examples/custom_distance/example_custom_distance.cpp |
Switches BVH construction to create_index and uses default query output mode. |
examples/brute_force/example_brute_force.cpp |
Removes local Iota helper and uses create_index for BVH/BruteForce. |
benchmarks/bvh_driver/benchmark_registration.hpp |
Uses create_index in benchmark tree construction. |
Suppressed comments (1)
src/spatial/ArborX_CrsGraphWrapper.hpp:78
- Same issue as the non-MPI overload:
IndexableGettercan deduce to an lvalue reference, causing the instantiated distributed index type to store a reference member for the getter. Decay the getter type when forming the Index template arguments to avoid dangling references and improve usability with lvalue views/getters.
return Index<MemorySpace, int, IndexableGetter>(
comm, space, Details::Iota<MemorySpace>(size),
std::forward<IndexableGetter>(indexable_getter));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return Index<MemorySpace, int, IndexableGetter>( | ||
| space, Details::Iota<MemorySpace>(size), | ||
| std::forward<IndexableGetter>(indexable_getter)); |
| auto bvh = ArborX::create_index<ArborX::BoundingVolumeHierarchy>( | ||
| space, points.size(), points); |
| Iota<typename TreeType::memory_space>{ | ||
| static_cast<int>(primitives.size())}, | ||
| primitives); | ||
| return ArborX::create_index<TreeType>(space, primitives.size(), primitives); |
| auto tree = ArborX::create_index<ArborX::BoundingVolumeHierarchy>( | ||
| ExecutionSpace{}, primitives.size(), | ||
| Kokkos::create_mirror_view_and_copy( |
Performant version of using
PairValueIndexis really challenging to grasp initially. For new users, this introduces too steep of a learning curve, potentially making people give up.Instead, it may make more sense to provide something much easier even if it does not have the best performance.
This PR introduces a new constructor to
BVHandBruteForce:This way, users will only have to write indexable getter, which should provide a lower barrier for entry.
Because we won't store the leaf nodes' geometries, it will result in a penalty during the traversal.
I'd like to hear the thoughts.