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

Add upper_bound kernel #1170

Merged
merged 5 commits into from
Oct 8, 2024
Merged

Add upper_bound kernel #1170

merged 5 commits into from
Oct 8, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Oct 3, 2024

  • Add KokkosExt::upper_bound kernel and tests
  • Move KokkosExt tests out of ArborX_Test_DetailsUtils

@aprokop aprokop added the enhancement New feature or request label Oct 3, 2024
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/tstDetailsKokkosExtKernelStdAlgorithms.cpp Outdated Show resolved Hide resolved
Kokkos::View<float *, Kokkos::HostSpace,
Kokkos::MemoryTraits<Kokkos::Unmanaged>>(v_ref.data(), n));
Kokkos::View<float *, DeviceType> v("Testing::v", n);
Kokkos::deep_copy(space, v, UnmanagedView<float>(v_ref.data(), n));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we care to copy with a specific exec space if we constructed the view without it?
Can't just use create_mirror_view_and_copy() ?

test/tstDetailsKokkosExtKernelStdAlgorithms.cpp Outdated Show resolved Hide resolved
test/tstDetailsKokkosExtKernelStdAlgorithms.cpp Outdated Show resolved Hide resolved
@aprokop aprokop force-pushed the upper_bound_kernel branch from cc99046 to 4d6da22 Compare October 4, 2024 19:35
@aprokop
Copy link
Contributor Author

aprokop commented Oct 4, 2024

Addressed the comments, and rebased on master to resolve conflict.

@aprokop aprokop requested a review from dalg24 October 8, 2024 18:02
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 have minor comments regarding the labelling. I would prefer these to be fixed prior to squash-merging.

space, v,
Kokkos::View<float *, Kokkos::HostSpace,
Kokkos::MemoryTraits<Kokkos::Unmanaged>>(v_ref.data(), n));
Kokkos::View<float *, DeviceType> v("Testing::v", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were going for "[ArborX]{Test,Example}::foo"

Copy link
Contributor Author

@aprokop aprokop Oct 8, 2024

Choose a reason for hiding this comment

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

We use "Testing::foo" and "Example::foo". We use ArborXTest namespaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please Testing (does not need to be resolved 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.

Please Testing (does not need to be resolved here

Are you asking to change all Testing:: to Test:: in a separate PR? Or to ArborXTest::?

@aprokop aprokop merged commit 97fcbc2 into arborx:master Oct 8, 2024
2 checks passed
@aprokop aprokop deleted the upper_bound_kernel branch October 8, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants