Move DefaultIndexableGetter to Experimental:: and provide CTAD for Indexables

[aprokop]

• Move DefaultIndexableGetter from Details:: to Experimental::
• Provide CTAD for Indexables

[aprokop]

@dalg24 Thoughts on this PR?

[dalg24]

Please elaborate on the rational/motivation
As I recall it, in Boost that function object was about being able to extract the geometry from things like pair or tuples but here it looks like a lateral move and it is unclear to me what this improves over the status quo as currently proposed

[aprokop]

Please elaborate on the rational/motivation

Move to Experimental::

This is straightforward. We can't keep it in Details when it is part of the official interface and is a default argument to the constructor.

Calling it Indexable

Better alignment with Boost.Geometry. Not a strong point, but there's little drawback for more compatibility.

Templating it on value type

This strong types it, so that the indexable getter will be guaranteed to work with only the provided value type.
I'm less confident about that, and deferred expertise to Boost.

[dalg24]

We can't keep it in Details when it is part of the official interface and is a default argument to the constructor.

Why not. We can say there is a default and define clearly what it does without making it something public. Users can deduce the type if they really want to spell it out.

[aprokop]

Why not. We can say there is a default and define clearly what it does without making it something public. Users can deduce the type if they really want to spell it out.

If a user wants to specify a custom bounding volume type for the hierarchy, they would have to write

   BVH<MemorySpace, Values, ArborX::Details::DefaultIndexableGetter, BoundingVolume> bvh(blah...);

thus putting Details:: namespace inside the user code.

[dalg24]

What benefit do we get from making it a template? Were you planing on supporting pair<Geometry, ...> or whatnot?

[aprokop]

What benefit do we get from making it a template?

That is a good question. I don't have a good answer other than Boost does it. And I trust them to make good interfaces more than myself. Can you think of anything?

Were you planing on supporting pair<Geometry, ...> or whatnot?

We already support PairValueIndex. In the previous version, it was one of the member functions, while in the new one it's partial specialization.

[aprokop]

I updated the PR. It no longer renames DefaultIndexableGetter and does not template it on the Value. Instead, it simply provides CTAD for Indexables and moves DefaultIndexableGetter from Details to Experimental.

[dalg24]

Looks good, just wanting to confirm that the Indexables deduction guide is needed

[dalg24]

Please remind me why we need to provide a user-defined deduction guide and it does not work out of the box.

[aprokop]

Implicitly-generated deduction guide for aggregates is C++20. See Notes section here.

[dalg24]

I would guard the deduction guides with #ifndef KOKKOS_ENABLE_CXX17 but leaving it up to you