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

Get rid of ArborX_DetailsKokkosExtMinMaxOperations.hpp #947

Closed
wants to merge 1 commit into from

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Sep 7, 2023

KokkosExt functions are a subset of the Kokkos functions.

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.

Remove the <ArborX_DetailsKokkosExtMinMaxOperations.hpp> header altogether.
All that functionality is available from Kokkos:: since version 3.7

@dalg24
Copy link
Contributor

dalg24 commented Sep 7, 2023

KokkosExt functions are a subset of the Kokkos functions. Their major limitation is that they break comparing two values of different types. This is particularly problematic. And resolving it in ArborX is challenging, given that this requires correct promotion of the arguments. half_t and bhalf_t would be particularly challenging.

There is nothing special about ArborX (nor Kokkos) implementation of min and max. If you pass two arguments of different types argument template deduction will fail. This is the same as with the standard library, just decorating the function templates with __host__ __device__ annotations.

The mere fact it is available in Kokkos is sufficient to argue for removal.
I don't understand why you are bringing up half types.

@aprokop
Copy link
Contributor Author

aprokop commented Sep 7, 2023

I don't understand why you are bringing up half types.

Maybe I misunderstood what Kokkos does. I was hoping that it would provide min(half_t, float) or similar. I now understand that C++ standard does not provide this. I'm not sure what would be the best way to do this then. The use case in point is having a Box<half_t> expanded by a Point<float>.

Kokkos supports all its functions since 3.7
@aprokop aprokop changed the title Remove KokkosExt::{min, max} in favor of Kokkos::{min, max} Get rid of ArborX_DetailsKokkosExtMinMaxOperations.hpp Sep 7, 2023
@dalg24
Copy link
Contributor

dalg24 commented Sep 8, 2023

One caveat with the Kokkos implementation is that we need to include <Kokkos_Core.hpp>. It is not obvious to me that it is a good change.

@aprokop
Copy link
Contributor Author

aprokop commented Sep 8, 2023

One caveat with the Kokkos implementation is that we need to include <Kokkos_Core.hpp>. It is not obvious to me that it is a good change.

I partially agree. I don't like that either, and would very much prefer that Kokkos_MinMaxClamp.hpp was a public header in Kokkos. Having said that, I think it's pretty safe to say that Kokkos_Core.hpp is included in almost every file in ArborX, one way or another, and definitely in every example and benchmark, and most tests. So I don't see how this changes the status quo.

@dalg24
Copy link
Contributor

dalg24 commented Sep 8, 2023

My point is that the maintenance burden for KokkosExt::{min,max} is low and that the current implementation in Kokkos does not provide any other advantage than we not having to define our own.

@aprokop aprokop closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants