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

std_algos: fix wrong corner case for is_partitioned #6257

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

fnrizzi
Copy link
Contributor

@fnrizzi fnrizzi commented Jul 4, 2023

Fix wrong behavior for corner case in Kokkos::Experimental::is_partitioned.

Currently, if we were to do:

template <class ValueType>
struct IsNegativeFunctor {
  KOKKOS_INLINE_FUNCTION
  bool operator()(const ValueType val) const { return (val < 0); }
};

Kokkos::View<int*> v = {11, 3, 17, 9, 3};
auto res = KE::is_partitioned(exespace(), KE::cbegin(v), KE::cend(v), p);

we get res == false.
However, this is wrong wrt std library see godbolt here and should return true.
This PR fixes this and the test that was wrongly set to false.

Note that the std library documentation https://en.cppreference.com/w/cpp/algorithm/is_partitioned is not super clear about this, in the sense that when none of the elements satisfy the predicate one could misinterpret it.

Comment: this bug was found when implementing the team-level api in #6256

fnrizzi added a commit to fnrizzi/kokkos that referenced this pull request Jul 4, 2023
@fnrizzi fnrizzi changed the title std_algos: fix wrong corner case for is_partitioned std_algos: fix wrong corner case for is_partitioned Jul 4, 2023
Copy link
Member

@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.

OK with the changes but not with the format disable

Copy link
Member

@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.

The disable format introduces inconsistencies (that look accidental) and it is not obvious to me what benefit we get from disabling.

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.

@Rombur
Copy link
Member

Rombur commented Jul 5, 2023

Retest this please

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