Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecate exclusivePrefixSum, accumulate, adjacentDifference, iota and provide KokkosExt ones #999

Merged
merged 16 commits into from
Dec 28, 2023

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Dec 26, 2023

ArborX::exclusivePrefixSum -> ArborX::Details::KokkosExt::exclusive_scan
ArborX::accumulate -> ArborX::Details::KokkosExt::reduce
ArborX::adjacentDifference -> ArborX::Details::KokkosExt::adjacent_difference
ArborX::iota -> ArborX::Details::KokkosExt::iota

Things I'm not sure about:

  • Should be do KokkosExt::StdAlgorithms namespace?
  • Should iota be in StdAlgorithms?

The goal of this and #998 is that ArborX_DetailsUtils.hpp is pretty clean post removal of deprecations, and nothing in ArborX:: namespace.

@aprokop aprokop added the refactoring Code reorganization label Dec 26, 2023
@aprokop aprokop changed the title Deprecate exclusivePrefixSum, accumulate, adjacentDiffernt and provide KokkosExt ones Deprecate exclusivePrefixSum, accumulate, adjacentDiffernce and provide KokkosExt ones Dec 26, 2023
@aprokop aprokop changed the title Deprecate exclusivePrefixSum, accumulate, adjacentDiffernce and provide KokkosExt ones Deprecate exclusivePrefixSum, accumulate, adjacentDiffernce, iota and provide KokkosExt ones Dec 26, 2023
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.

Why did you not introduce the initial value trailing argument with exclusive_scan if the intent is to align with standard parallel algorithms?

@aprokop
Copy link
Contributor Author

aprokop commented Dec 27, 2023

Why did you not introduce the initial value trailing argument with exclusive_scan if the intent is to align with standard parallel algorithms?

Right now, the only thing I've done is moving things around. I haven't done the alignment with the standard algorithms in this PR.
Added initial value.

@masterleinad
Copy link
Collaborator

Things I'm not sure about:

  • Should be do KokkosExt::StdAlgorithms namespace?
  • Should iota be in StdAlgorithms?

I don't think either of these are necessary (but it's easy to change since we don't care about backward-compatibility in that namespace).

@aprokop aprokop mentioned this pull request Dec 27, 2023
@aprokop aprokop changed the title Deprecate exclusivePrefixSum, accumulate, adjacentDiffernce, iota and provide KokkosExt ones Deprecate exclusivePrefixSum, accumulate, adjacentDifference, iota and provide KokkosExt ones Dec 27, 2023

Kokkos::View<int[6], DeviceType> v("v");
Kokkos::deep_copy(v, 5);
BOOST_TEST(KokkosExt::reduce(space, v, 3) == 33);
Copy link
Contributor

Choose a reason for hiding this comment

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

add a using-declaration and do unqualified call here and below

  using ArborX::Details::KokkosExt::reduce;

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 prefer the way I did it.


BOOST_AUTO_TEST_CASE_TEMPLATE(exclusive_scan, DeviceType, ARBORX_DEVICE_TYPES)
{
namespace KokkosExt = ArborX::Details::KokkosExt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefet

Suggested change
namespace KokkosExt = ArborX::Details::KokkosExt;
using ArborX::Details::KokkosExt::exclusive_scan;

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.

I would really like you to switch to using declaration in the unit test but I am not blocking

@aprokop aprokop merged commit b338368 into arborx:master Dec 28, 2023
@aprokop aprokop deleted the std_algorithms branch December 28, 2023 16:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants