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

Implement Kokkos::Experimental::{min,max,minmax}(std::initializer_list) #4629

Merged
merged 5 commits into from
Dec 22, 2021

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Dec 21, 2021

Rational: reviewed some code downstream that was doing things like min(a, min(b, c)

Make sure you see the drib-by fixing of a defect in operator== for Kokkos::pair<T1, T2> that was missing the constexpr specifier.
About the overloads I am adding, I just followed closely the design of std::{min,max,minmax} (see https://eel.is/c++draft/alg.min.max)

@dalg24 dalg24 requested a review from fnrizzi December 21, 2021 18:54
@dalg24 dalg24 changed the title Implement Kokkos::Experimental{min,max,minmax}(std::initializer_list) Implement Kokkos::Experimental::{min,max,minmax}(std::initializer_list) Dec 21, 2021
@fnrizzi
Copy link
Contributor

fnrizzi commented Dec 21, 2021

@dalg24 one thing: minmax in the std is supposed to do this: " If several elements are equivalent to the smallest, the leftmost such element is returned. If several elements are equivalent to the largest, the rightmost such element is returned."
I saw that in your minmax tests you don't check for this but only that the values found are the smallest and max.
I think this is fine because minmax returns the values not iterators.

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.

Looks good to me.

@dalg24
Copy link
Member Author

dalg24 commented Dec 21, 2021

@dalg24 one thing: minmax in the std is supposed to do this: " If several elements are equivalent to the smallest, the leftmost such element is returned. If several elements are equivalent to the largest, the rightmost such element is returned."
I saw that in your minmax tests you don't check for this but only that the values found are the smallest and max.
I think this is fine because minmax returns the values not iterators.

I overlooked it. I think actually this means that my max and my minmax do not follow the standard specs.

Copy link
Contributor

@fnrizzi fnrizzi left a comment

Choose a reason for hiding this comment

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

as discussed, i think we need to verify/fix behavior

@masterleinad
Copy link
Contributor

Surprisingly, a (different) OpenMPTarget test is failing:

[ RUN      ] openmptarget.unique_token_global
4: /var/jenkins/workspace/Kokkos/core/unit_test/TestUniqueToken.hpp:151: Failure
4: Expected equality of these values:
4:   sum
4:     Which is: 9999990
4:   int64_t(N) * R
4:     Which is: 10000000
4: [  FAILED  ] openmptarget.unique_token_global (261 ms)

@dalg24
Copy link
Member Author

dalg24 commented Dec 21, 2021

For reference it looks like implementations of the standard library compute the same thing as I did https://godbolt.org/z/qP5Tv3jds

@fnrizzi
Copy link
Contributor

fnrizzi commented Dec 22, 2021

@dalg24
There is a difference in how max behaves when it is by itself vs when it is part of minmax.

  • std::min(std::initializer_list):

    • If several values are equivalent to the smallest, returns the LEFTMOST one
  • std::max(std::initializer_list):

    • If several values are equivalent to the greatest, returns the LEFTMOST one
  • std::minmax(std::initializer_list):

    • If several elements are equivalent to the SMALLEST, the leftmost is returned.
    • If several elements are equivalent to the LARGEST, the rightmost such element is returned.

so min does not change, but max does. See this: https://godbolt.org/z/9EohjbsGT

Copy link
Contributor

@fnrizzi fnrizzi left a comment

Choose a reason for hiding this comment

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

thanks for getting it through :)

@dalg24 dalg24 merged commit 2738767 into kokkos:develop Dec 22, 2021
@dalg24 dalg24 deleted the min_max_initializer_list branch December 22, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants