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 requirement on knowing geometry types in IndexableGetter #1211

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Feb 5, 2025

In the current iteration, if a user has a custom geometry type that they try to build hierarchy on, it has to be tagged with one of the ArborX tags, which would call ArborX routines. This patch relaxes this requirement. Now, a user only needs to specify dimension and coordinate type.

I added a compile-only test that shows the requirements.

@aprokop aprokop added the API User visible interface modifications label Feb 5, 2025
Comment on lines +37 to +38
typename Enable = std::enable_if_t<
!Details::is_pair_value_index_v<std::decay_t<Geometry>>>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed so that universal reference does not grab PairValueIndex specializations.

@aprokop aprokop requested a review from dalg24 February 5, 2025 15:39
src/spatial/detail/ArborX_PairValueIndex.hpp Show resolved Hide resolved
test/tstCompileOnlyTypeRequirements.cpp Outdated Show resolved Hide resolved
test/tstCompileOnlyTypeRequirements.cpp Outdated Show resolved Hide resolved
KOKKOS_FUNCTION Value operator()(PairValueIndex<Value, Index> &&pair) const
template <typename Geometry, typename Index>
KOKKOS_FUNCTION Geometry
operator()(PairValueIndex<Geometry, Index> &&pair) const
{
return pair.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be moving it and returning a && here?

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'm not sure why you are asking. We return a struct member. We don't need to do anything with the parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we don't care since we get guaranteed copy elision?

@aprokop aprokop force-pushed the update_indexable_getter branch from 3c34dfb to 6ffdcf2 Compare February 5, 2025 16:25
@aprokop
Copy link
Contributor Author

aprokop commented Feb 5, 2025

Couple irrelevant build failures (out of space, gpu not picked up).

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.

2 participants