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 Kokkos::reduction_identity into its own header file #5450

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Sep 9, 2022

Move reduction_identity from <Kokkos_NumericTraits.hpp> to a new (public?) header <Kokkos_ReductionIdentity.hpp>

Admittedly this may break code downstream :/

@dalg24 dalg24 added the Refactor Tidying up: make code code, cleaner, simpler, understandable and robust label Sep 9, 2022
Copy link
Contributor

@JBludau JBludau left a comment

Choose a reason for hiding this comment

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

looks good.

@dalg24 dalg24 marked this pull request as ready for review September 19, 2022 22:01
masterleinad
masterleinad previously approved these changes Sep 19, 2022
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.

Let's see if someone complains. In the end, reduction_identity should only be relevant if people call parallel_reduce but, of course, they might just have included this header for specializing reduction_identity. Anyway, if we merge early, we can hope that users complain early and it would be easy to revert.

@dalg24 dalg24 dismissed masterleinad’s stale review September 20, 2022 01:04

Not sure I was clear enough

@dalg24
Copy link
Member Author

dalg24 commented Sep 20, 2022

Let's see if someone complains. In the end, reduction_identity should only be relevant if people call parallel_reduce but, of course, they might just have included this header for specializing reduction_identity. Anyway, if we merge early, we can hope that users complain early and it would be easy to revert.

I was not clear enough. We know that it will break code downstream (for instance KokkosKernels and ArborX). If we decide to merge it, we are not doing it to see if people complain loud enough and revert, we expect them to update their code.
The main question is do we agree that it is the right thing to do.

edit actually I looked closer and it wouldn't actually break KokkosKernels. The source files that use Kokkos::reduction_identity include <Kokkos_Core.hpp>

@masterleinad
Copy link
Contributor

If this breaks too many projects, we could, of course, include ReductionIdentity.hpp in Kokkos_NumericTraits.hpp again.

@dalg24
Copy link
Member Author

dalg24 commented Sep 20, 2022

If this breaks too many projects, we could, of course, include ReductionIdentity.hpp in Kokkos_NumericTraits.hpp again.

I would only consider it if guarded with KOKKOS_ENABLE_DEPRECATED_CODE_4

@Rombur
Copy link
Member

Rombur commented Sep 21, 2022

I think it's a good change but we should deprecate the current code since we know it will break user code.

@dalg24
Copy link
Member Author

dalg24 commented Sep 21, 2022

I think it's a good change but we should deprecate the current code since we know it will break user code.

OK as a compromise what if I include the new <Kokkos_ReductionIdentity.hpp> header from <Kokkos_NumericTraits.hpp> (96a9c76) when Kokkos_ENABLE_DEPRECATED_CODE_4 is ON

@dalg24 dalg24 merged commit 153a39e into kokkos:develop Sep 21, 2022
@dalg24 dalg24 deleted the move_reduction_identity branch September 21, 2022 19:08
@dalg24 dalg24 mentioned this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Tidying up: make code code, cleaner, simpler, understandable and robust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants