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.

Sign up for GitHub

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

Jump to bottom

[READY] move min, max, minmax, clamp to math functions #4425 #4506

Merged
merged 4 commits into from
Nov 11, 2021
Merged

[READY] move min, max, minmax, clamp to math functions #4425 #4506

merged 4 commits into from
Nov 11, 2021

Conversation

fnrizzi
Copy link
Contributor

@fnrizzi fnrizzi commented Nov 8, 2021

addresses #4425

@fnrizzi fnrizzi changed the title move min, max, minmax, clamp to math functions #4425 [WIP] move min, max, minmax, clamp to math functions #4425 Nov 8, 2021
@fnrizzi
Copy link
Contributor Author

fnrizzi commented Nov 8, 2021 •
edited

@dalg24 is Kokkos_MathematicalFunctions.hpp the right place to move these functions to? I kept everything under experimental as agreed on.

@dalg24
Copy link
Member

dalg24 commented Nov 8, 2021

@dalg24 is Kokkos_MathematicalFunctions.hpp the right place to move these functions to? I kept everything under experimental as agreed on.

I'd keep them in their own header like you had them before.

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Nov 8, 2021

@dalg24 ok, thanks, but in their own header inside core correct?

@dalg24
Copy link
Member

dalg24 commented Nov 8, 2021

@dalg24 ok, thanks, but in their own header inside core correct?

Yes

dalg24
dalg24 reviewed Nov 8, 2021
View reviewed changes
core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
algorithms/src/Kokkos_StdAlgorithms.hpp Show resolved Hide resolved
@fnrizzi fnrizzi requested a review from cz4rs November 8, 2021 16:07
cz4rs
cz4rs approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@cz4rs cz4rs 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 to me!

@cz4rs
Copy link
Contributor

cz4rs commented Nov 9, 2021

I think this should be good to go once #4509 is merged.

@masterleinad
Copy link
Contributor

Should the title still contain "[WIP]"?

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Nov 10, 2021 •
edited

Should the title still contain "[WIP]"?

@masterleinad thanks for the message! I left it there because I saw CI partially failing, so was waiting to see if was something caused by the PR. I think I can remove the WIP since it seems it is not that.

@fnrizzi fnrizzi added this to In progress in Kokkos Release 3.6 via automation Nov 10, 2021
@fnrizzi fnrizzi added this to In progress in Developer: FRANCESCO RIZZI Nov 10, 2021
@fnrizzi fnrizzi changed the title [WIP] move min, max, minmax, clamp to math functions #4425 [READY] move min, max, minmax, clamp to math functions #4425 Nov 10, 2021
@dalg24
Copy link
Member

dalg24 commented Nov 10, 2021

Retest this please

@crtrott crtrott merged commit 16e2b9a into kokkos:develop Nov 11, 2021
Kokkos Release 3.6 automation moved this from In progress to Done Nov 11, 2021
@fnrizzi fnrizzi moved this from In progress to Done in Developer: FRANCESCO RIZZI Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

@cz4rs cz4rs cz4rs approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Developer: FRANCESCO RIZZI
Done
Kokkos Release 3.6
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fnrizzi @dalg24 @cz4rs @masterleinad @crtrott