-
-
Notifications
You must be signed in to change notification settings - Fork 542
[parallel] Fix hpx::nth_element to correctly support C++20 projections and sentinels #7191
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
aneek22112007-tech
wants to merge
18
commits into
TheHPXProject:master
Choose a base branch
from
aneek22112007-tech:fix/nth-element-projections-sentinels
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+93
−13
Open
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
5fd7338
[parallel] Fix hpx::nth_element to correctly support projections and …
dasaneek007-cpu 181ec0a
Apply clang-format fixes
dasaneek007-cpu f895d19
Merge branch 'master' into fix/nth-element-projections-sentinels
aneek22112007-tech b0ebd62
Remove <iostream> inclusion to fix hpxinspect failure
dasaneek007-cpu 9740280
Revert hpx::nth_element to standard compliance, move projections/sent…
dasaneek007-cpu e58f6aa
Address maintainer feedback: consolidate wrapped_comp_type and use st…
dasaneek007-cpu 8199058
Apply clang-format fixes
dasaneek007-cpu 5590bc1
Fix macOS CI: avoid detail::min_element type-matching issue with proj…
dasaneek007-cpu 891e829
Merge branch 'master' into fix/nth-element-projections-sentinels
aneek22112007-tech 5f9a80b
Fix type deduction in minmax.hpp and remove workaround in nth_element…
dasaneek007-cpu e61e974
Apply clang-format corrections to minmax.hpp
dasaneek007-cpu 3ad02be
Fix clang-format in minmax.hpp after CI failure
dasaneek007-cpu c860663
Fix clang-format in minmax.hpp (final attempt)
dasaneek007-cpu e993817
Automated clang-format fix using opt/homebrew/bin/clang-format
dasaneek007-cpu 30cf465
Refine element_type deduction to handle HPX proxies using proxy_value_t
dasaneek007-cpu 0e2e53e
Remove redundant typename keywords as requested by maintainer
dasaneek007-cpu 865d418
Restore typename keywords (strictly required for CI builds)
dasaneek007-cpu a4b4a86
Separate core fix into independent PR and restore temporary workaround
dasaneek007-cpu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
88 changes: 88 additions & 0 deletions
88
libs/core/algorithms/tests/unit/algorithms/nth_element_projection.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| // Copyright (c) 2014 Grant Mercer | ||
| // Copyright (c) 2017-2024 Hartmut Kaiser | ||
| // | ||
| // SPDX-License-Identifier: BSL-1.0 | ||
| // Distributed under the Boost Software License, Version 1.0. (See accompanying | ||
| // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| #include <hpx/algorithm.hpp> | ||
| #include <hpx/init.hpp> | ||
| #include <hpx/modules/testing.hpp> | ||
|
|
||
| #include <cstddef> | ||
| #include <iostream> | ||
| #include <numeric> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| struct S | ||
| { | ||
| int val; | ||
| }; | ||
|
|
||
| template <typename ExPolicy> | ||
| void test_nth_element_projection(ExPolicy policy) | ||
| { | ||
| std::vector<S> v(100); | ||
| for (int i = 0; i < 100; ++i) | ||
| v[i].val = 100 - i; | ||
|
|
||
| auto nth = v.begin() + 50; | ||
|
|
||
| hpx::nth_element( | ||
| policy, v.begin(), nth, v.end(), std::less<int>{}, &S::val); | ||
|
|
||
| // After nth_element, the element at nth should be the 51st smallest element (which is 51) | ||
| HPX_TEST_EQ(v[50].val, 51); | ||
|
|
||
| // All elements before nth should be <= v[50].val | ||
| for (auto it = v.begin(); it != nth; ++it) | ||
| { | ||
| HPX_TEST_LTE(it->val, v[50].val); | ||
| } | ||
|
|
||
| // All elements after nth should be >= v[50].val | ||
| for (auto it = nth + 1; it != v.end(); ++it) | ||
| { | ||
| HPX_TEST(it->val >= v[50].val); | ||
| } | ||
| } | ||
|
|
||
| template <typename ExPolicy> | ||
| void test_nth_element_projection_type_change(ExPolicy policy) | ||
| { | ||
| std::vector<std::string> v = {"abc", "a", "abcd", "ab", "abcde"}; | ||
| auto nth = v.begin() + 2; | ||
|
|
||
| // Sort by string length | ||
| hpx::nth_element(policy, v.begin(), nth, v.end(), std::less<std::size_t>{}, | ||
| &std::string::length); | ||
|
|
||
| HPX_TEST_EQ(v[2].length(), std::size_t(3)); | ||
| } | ||
|
|
||
| int hpx_main() | ||
| { | ||
| test_nth_element_projection(hpx::execution::seq); | ||
| test_nth_element_projection(hpx::execution::par); | ||
| test_nth_element_projection(hpx::execution::par_unseq); | ||
|
|
||
| test_nth_element_projection_type_change(hpx::execution::seq); | ||
| test_nth_element_projection_type_change(hpx::execution::par); | ||
| test_nth_element_projection_type_change(hpx::execution::par_unseq); | ||
|
|
||
| return hpx::local::finalize(); | ||
| } | ||
|
|
||
| int main(int argc, char* argv[]) | ||
| { | ||
| std::vector<std::string> const cfg = {"hpx.os_threads=all"}; | ||
|
|
||
| hpx::local::init_params init_args; | ||
| init_args.cfg = cfg; | ||
|
|
||
| HPX_TEST_EQ_MSG(hpx::local::init(hpx_main, argc, argv, init_args), 0, | ||
| "HPX main exited with non-zero status"); | ||
|
|
||
| return hpx::util::report_errors(); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale of this change?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the underlying detail::min_element implementation in minmax.hpp actually fails on macOS Clang when a projection changes the value type (it tries to store the result in a variable of the original element type). Using the wrapped comparison here is a necessary workaround to keep the macOS builds green.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to fix the actual problem instead of adding a workaround here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to fix this problem (in an independent PR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I've followed your suggestion and separated the core fix for minmax.hpp into an independent Pull Request. You can find it here: #7208.
On this current branch, I have:
Reverted the changes to minmax.hpp to match master.
Restored the wrapped_comp_type workaround in nth_element.hpp specifically for the min_element call.
This keeps this PR functional and 'Green' in CI while the core fix is being reviewed. Once the independent minmax PR is merged, I will come back here to remove the temporary workaround. Let me know if that works for you!