Skip to content

Add semantic tests for extrema algorithms with repeated values #6684

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

harith-hacky03
Copy link
Contributor

Fixes #6627

Proposed Changes

  • Added semantic tests for extrema algorithms (min_element, max_element, minmax_element) to verify behavior with repeated values
  • Tests ensure that:
    • min_element returns the first occurrence of the minimum value
    • max_element returns the first occurrence of the maximum value
    • minmax_element returns the first occurrence of the minimum value and the last occurrence of the maximum value
  • Added tests for all execution policies (seq, par, par_unseq) and custom comparison functions

Any background context you want to provide?

The existing tests for extrema algorithms did not cover the important semantic behavior when dealing with repeated values. These new tests ensure that the algorithms maintain their expected behavior across different execution policies and comparison functions.

Checklist

  • I have added a new feature and have added tests to go along with it.
  • I have fixed a bug and have added a regression test.
  • I have added a test using random numbers; I have made sure it uses a seed, and that random numbers generated are valid inputs for the tests.

@harith-hacky03 harith-hacky03 requested a review from hkaiser as a code owner April 20, 2025 04:45
@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch from 5ab364e to 2b53e19 Compare April 20, 2025 04:47
@StellarBot
Copy link

Can one of the admins verify this patch?

Copy link

codacy-production bot commented Apr 20, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.07% 98.65%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (8ffc778) 196632 167501 85.19%
Head commit (d7f5933) 193821 (-2811) 164979 (-2522) 85.12% (-0.07%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6684) 148 146 98.65%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@@ -7,6 +7,7 @@
#include <hpx/algorithm.hpp>
#include <hpx/init.hpp>
#include <hpx/modules/testing.hpp>
#include <hpx/parallel/algorithms/max_element.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this header file #included implicitly by <hpx/algorithm.hpp>?

@@ -7,6 +7,7 @@
#include <hpx/algorithm.hpp>
#include <hpx/init.hpp>
#include <hpx/modules/testing.hpp>
#include <hpx/parallel/algorithms/min_element.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this header file #included implicitly by <hpx/algorithm.hpp>?

@@ -7,6 +7,7 @@
#include <hpx/algorithm.hpp>
#include <hpx/init.hpp>
#include <hpx/modules/testing.hpp>
#include <hpx/parallel/algorithms/minmax_element.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this header file #included implicitly by <hpx/algorithm.hpp>?

@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch 2 times, most recently from 20f30ea to b98e3ea Compare April 24, 2025 03:50
@hkaiser
Copy link
Member

hkaiser commented Apr 27, 2025

@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch from 3581f34 to a01e4e1 Compare April 27, 2025 03:46
@harith-hacky03
Copy link
Contributor Author

@hkaiser What's the expected behaviour of max_element if i have multiple max elements?

@hkaiser
Copy link
Member

hkaiser commented Apr 27, 2025

@hkaiser What's the expected behaviour of max_element if i have multiple max elements?

See: https://en.cppreference.com/w/cpp/algorithm/max_element

Return value
Iterator to the greatest element in the range [first, last). If several elements in the range are equivalent to the greatest element, returns the iterator to the first such element. Returns last if the range is empty.

@hkaiser
Copy link
Member

hkaiser commented Apr 27, 2025

@harith-hacky03 please squash your commits and also remove all unrelated changes from this PR.

@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch 2 times, most recently from 2b59d9b to 8cdb1d5 Compare April 27, 2025 14:59
@harith-hacky03
Copy link
Contributor Author

@hkaiser Don't know why am i getting those commits

…t, max_element, and minmax_element

Signed-off-by: harith-hacky03 <[email protected]>
@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch from 1800813 to ea79800 Compare April 27, 2025 15:12
…t, max_element, and minmax_element

Signed-off-by: harith-hacky03 <[email protected]>
@harith-hacky03 harith-hacky03 force-pushed the feature/extrema-algorithms-semantics branch from ea79800 to 9755e1a Compare April 27, 2025 15:16
Copy link

codacy-production bot commented Apr 27, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.65% 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (a31463c) 243218 208594 85.76%
Head commit (9755e1a) 193832 (-49386) 164979 (-43615) 85.11% (-0.65%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6684) 20 20 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@@ -771,7 +779,7 @@ namespace hpx::parallel {

element_type curr_max_value =
HPX_INVOKE(proj, *curr->max);
if (!HPX_INVOKE(f, curr_max_value, max_value))
if (HPX_INVOKE(f, curr_max_value, max_value))
Copy link
Member

Choose a reason for hiding this comment

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

This is now inconsistent with your sequential implementation above (same for the change below). Which version is correct?

@hkaiser
Copy link
Member

hkaiser commented May 9, 2025

@harith-hacky03 ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extrema algorithms with repeated values
3 participants