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

Move min, max, clamp, minmax routines out of Experimental namespace #5170

Conversation

ajpowelsnl
Copy link
Contributor

@ajpowelsnl ajpowelsnl commented Jun 30, 2022

Fixes #5168.

I propose moving the min, max, clamp and minmax routines out of the Experimental namespace, and into the Kokkos namespace.

I developed these changes using this build:

cmake \
-DKokkos_ENABLE_TESTS=ON \
-DKokkos_ENABLE_CUDA=ON \
-DKokkos_ENABLE_CUDA_LAMBDA=ON \
-DKokkos_ARCH_VOLTA70=ON \
-DKokkos_ENABLE_SERIAL=ON \
-DCMAKE_CXX_STANDARD=14 \
-DCMAKE_BUILD_TYPE=Release ..\

I ran all unit tests thusly:

ctest --output-on-failure

All tests passed.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

We should rather deprecate using the functions in the Experimental namespace rather than outright removing them.
Also, you need to fix the indentation using clang-format-8.

@fnrizzi
Copy link
Contributor

fnrizzi commented Jul 1, 2022

I agree on deprecating

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 7, 2022
@crtrott crtrott force-pushed the update/move_min_max_from_Experimental_namespace branch from 96a9cba to c9e065a Compare July 7, 2022 20:14
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

The only disadvantage with this approach I see is that we don't get deprecation warnings. Of course, we followed the same approach when promoting the math functions from the Experimental namespace.

@masterleinad masterleinad dismissed their stale review July 7, 2022 21:05

Crucial changes were made.

@fnrizzi
Copy link
Contributor

fnrizzi commented Jul 8, 2022

if this is how the math functins were promoted, i am ok with this

@crtrott crtrott merged commit c18359d into kokkos:develop Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants