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

Simplify using APIv1 through APIv2 interface #1051

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Apr 1, 2024

Right now, it is inconvenient to get the old behavior. One has to write

ArborX::BVH<MemorySpace, ArborX::PairValueIndex<Box>>
  tree(space, ArborX::Experimental::attach_indices(boxes));
tree.query(space, queries, ArborX::LegacyDefaultCallback{}, indices, offsets);

The presence of the LegacyDefaultCallback is really annoying.

This patch changes this. It automatically adds LegacyDefaultCallback
if the following 3 conditions are satisfied:

  1. A user does not provide a callback
  2. The index is constructed on PairValueIndex
  3. The output value_type matches the index_type in the PairValueIndex.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 1, 2024

@dalg24 Thoughts?

@aprokop aprokop added the API User visible interface modifications label Apr 1, 2024
@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 1284e65 to 7e80566 Compare April 8, 2024 20:24
Right now, it is inconvenient to get the old behavior. One has to write
```c++
ArborX::BVH<MemorySpace, ArborX::PairValueIndex<Box>>
  tree(space, ArborX::Experimental::attach_indices(boxes));
tree.query(space, queries, ArborX::LegacyDefaultCallback{}, indices, offsets);
```
The presence of the `LegacyDefaultCallback` is really annoying.

This patch changes this. It automatically adds LegacyDefaultCallback
if the following 3 conditions are satisfied:
1. A user does not provide a callback
2. The index is constructed on PairValueIndex
2. The output value_type matches the index_type in the PairValueIndex.
@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 7e80566 to 3030aa8 Compare April 8, 2024 20:30
@aprokop aprokop marked this pull request as ready for review April 8, 2024 20:30
@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 3030aa8 to 1cf1a0d Compare April 8, 2024 20:31
@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 1cf1a0d to b257854 Compare April 8, 2024 20:49
@aprokop aprokop added this to the Tentative 1.6 Release milestone Apr 9, 2024
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.

Did you try to deduce the integral type to use instead of only enabling for unsigned

@aprokop
Copy link
Contributor Author

aprokop commented Apr 9, 2024

Did you try to deduce the integral type to use instead of only enabling for unsigned

I didn't enable it only for unsigned. I enabled it for the case where the index type stored in PairValueIndex matches the view's value_type provided by the user. Could be both unsigned or int. I decided not to do any implicit conversions.

As the user is the one in control for both types, we cannot do anything on our side imho.

@aprokop aprokop mentioned this pull request Apr 10, 2024
@dalg24
Copy link
Contributor

dalg24 commented Apr 11, 2024

Exactly I was thinking about doing the conversion. Why do you think we shouldn't?

@aprokop
Copy link
Contributor Author

aprokop commented Apr 11, 2024

Exactly I was thinking about doing the conversion. Why do you think we shouldn't?

I am concerned about losing precision. What if the hierarchy is created on PairValueIndex<Geometry, unsigned> and user requested Kokkos::View<short*>?

Converting unsigned -> int is maybe ok. It would be nice to make user to explicitly allow enable conversion.

@dalg24
Copy link
Contributor

dalg24 commented Apr 11, 2024

We say that all attached indexes being representable as the integral value type of the view is a precondition

@aprokop
Copy link
Contributor Author

aprokop commented Apr 11, 2024

We say that all attached indexes being representable as the integral value type of the view is a precondition

You mean in the documentation? This feels like prone to subtle errors.
And still won't solve (unsigned, int) pair. Unless you are talking about actually stored values and not the ranges.

Could theoretically use something like std::in_range (I know, C++20) for individual values careful conversion. Not sure about the performance cost.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 11, 2024

After a discussion, we agreed to relax the requirements on type matching, and simply require that the value type in the output view is integral.

Rationale: we already do if for the query(space, predicates, indices, output) in the legacy format, and the output time does not have to match.

@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 2df3a20 to 92d5e96 Compare April 11, 2024 16:02
src/ArborX_LinearBVH.hpp Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_BruteForce.hpp Outdated Show resolved Hide resolved
@aprokop aprokop force-pushed the simplify_apiv2_in_apiv1_mode branch from 7432c99 to 9a32bf4 Compare April 11, 2024 18:12
@aprokop aprokop merged commit 5cae9f6 into arborx:master Apr 11, 2024
2 checks passed
@aprokop aprokop deleted the simplify_apiv2_in_apiv1_mode branch April 11, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API User visible interface modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants