-
Notifications
You must be signed in to change notification settings - Fork 407
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
make constraints on Kokkos::sort
more visible/clear
#6234
Conversation
Kokkos::sort
more visible/clear
Please remind me why we care about sorting |
I actually had no idea, and found out that it was supporting that when doing this work ... if i had to guess, it comes from lammps or maybe maybe something else particle-based. Even if lammps does not call kokkos sort directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I am OK with pulling the <Kokkos_DynamicView.hpp>
header into <Kokkos_Sort.hpp>
in the name of better compile error diagnostic.
@masterleinad I made all changes you requested |
@@ -219,6 +258,10 @@ std::enable_if_t<Kokkos::is_execution_space<ExecutionSpace>::value> sort( | |||
|
|||
template <class ViewType> | |||
void sort(ViewType view, size_t const begin, size_t const end) { | |||
// same constraints as the overload above which this gets dispatched to | |||
static_assert(ViewType::rank == 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all need to call the member function rank()
if DynRankView
if rank 1 is to be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't work for binsort beause of this: https://github.com/kokkos/kokkos/blob/develop/algorithms/src/sorting/impl/Kokkos_CopyOpsForBinSortImpl.hpp
Some of the constraints on the sort functions were a bit hidden: for example some overloads accept a dynamic view, while some others don't. This PR clarifies directly at the API level what the constraints are on the current sort API, thus making it clearly visible right away (aka one does not have to dig a few layers to figure out what is allowed)
Minor: clang format is turned off so that the the public API is more readable (IMMO)