Changed reduce implementation and added tests

[BhoomishGupta]

Fixes #6647

Proposed Changes

Created a helper function that reduces the partition value without the need of initial value..
Added a test case that was mentioned in the issue #6647.
Corrected a typo in documentation.

I only did changes in :

1. hpx\libs\core\algorithms\include\hpx\parallel\algorithms\reduce.hpp
2. hpx\libs\core\algorithms\tests\regressions\CMakeLists.txt
3. the rest were changes due to clang and editor.config

And created this :

1. hpx\libs\core\algorithms\tests\regressions\reduce_6647.cpp

Any background context you want to provide?

Previously, the implementation assumed that the *first was convertible to T. This assumption does not hold for the actual function logic required here. This PR removes that dependency, ensuring the type handling is correct.

Checklist

Not all points below apply to all pull requests.

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

[StellarBot]

Can one of the admins verify this patch?

[hkaiser]

Could you please separate the formatting changes into a different PR? It's close to impossible to review your actual changes here.

Also, for the formatting - are you sure you're using clang-format V20?

[BhoomishGupta]

Yeah, I was not using V20.
So, should I close this PR and open a new one, that is only with the changes?

[hkaiser]

Yeah, I was not using V20. So, should I close this PR and open a new one, that is only with the changes?

Simply keep (force) pushing to your branch, this wil update the PR.

[hkaiser]

@BhoomishGupta FWIW, the reduce tests now seem to fail. Also, please rebase thise onto master to resolve the conflicts.

[BhoomishGupta]

@BhoomishGupta FWIW, the reduce tests now seem to fail. Also, please rebase thise onto master to resolve the conflicts.

Working on it

[hkaiser]

@BhoomishGupta Please rebase your changes onto master to reduce the amount of unrelated changes (currently more than 300 files). Otherwise it's close to impossible to review your PR.

[hkaiser]

@BhoomishGupta Thanks for investigating and attempting to implement a new customization point for the execution parameters.

You have implemented part of the necessary logic. Please have a look at the existing customization points to see what's necessary additionally.

I have the impression, that you have added the new functionality for all algorithms, as the new functionality will be invoked always. I'd suggest making sure, that the new functionality is used only for the reduction algorithms.

[BhoomishGupta]

@BhoomishGupta Thanks for investigating and attempting to implement a new customization point for the execution parameters.

You have implemented part of the necessary logic. Please have a look at the existing customization points to see what's necessary additionally.

I have the impression, that you have added the new functionality for all algorithms, as the new functionality will be invoked always. I'd suggest making sure, that the new functionality is used only for the reduction algorithms.

@hkaiser Thanks for the review. I will check the existing customization points and add the missing things.

Regarding the scope: I did intentionally add it to the generic utility because I thought adjusting chunk sizes could be beneficial for other algorithms down the line. Or do you strongly prefer I hard-restrict it to reduce only?

Just to clarify on the current behavior: even though it's in a shared file, it currently acts as a no-op for other algorithms. The default implementation returns {0, 0}, and the call sites only apply adjustments when the return values are non-zero. So it shouldn't be impacting non-reduce algorithms right now, but I will make sure the trait-detection aligns perfectly with the rest of the codebase.

[hkaiser]

@hkaiser Thanks for the review. I will check the existing customization points and add the missing things.

Regarding the scope: I did intentionally add it to the generic utility because I thought adjusting chunk sizes could be beneficial for other algorithms down the line. Or do you strongly prefer I hard-restrict it to reduce only?

The default behavior is needed for all algorithms (as a fall-back, most likely), while the customization should apply to the reduction algorithms only.

Just to clarify on the current behavior: even though it's in a shared file, it currently acts as a no-op for other algorithms. The default implementation returns {0, 0}, and the call sites only apply adjustments when the return values are non-zero. So it shouldn't be impacting non-reduce algorithms right now, but I will make sure the trait-detection aligns perfectly with the rest of the codebase.

If the default used by all algorithms (except reduce) is now {0, 0}, wouldn't that change the current behavior?

[BhoomishGupta]

@hkaiser Thanks for the review. I will check the existing customization points and add the missing things.
Regarding the scope: I did intentionally add it to the generic utility because I thought adjusting chunk sizes could be beneficial for other algorithms down the line. Or do you strongly prefer I hard-restrict it to reduce only?

The default behavior is needed for all algorithms (as a fall-back, most likely), while the customization should apply to the reduction algorithms only.

Just to clarify on the current behavior: even though it's in a shared file, it currently acts as a no-op for other algorithms. The default implementation returns {0, 0}, and the call sites only apply adjustments when the return values are non-zero. So it shouldn't be impacting non-reduce algorithms right now, but I will make sure the trait-detection aligns perfectly with the rest of the codebase.

If the default used by all algorithms (except reduce) is now {0, 0}, wouldn't that change the current behavior?

I don,t think that the {0, 0} default changes the current behavior for any algorithm.

The call site in chunk_size.hpp

auto [adj_chunk, adj_max] =
    hpx::execution::experimental::adjust_chunk_size_and_max_chunks(
        policy.parameters(), policy.executor(),
        max_chunks, chunk_size);
if (adj_chunk != 0)
    chunk_size = adj_chunk;
if (adj_max != 0)
    max_chunks = adj_max;
else if (adj_chunk == 0)
    adjust_chunk_size_and_max_chunks(
        cores, count, max_chunks, chunk_size);

works when the Customization Point returns the default {0, 0}, both adj_chunk and adj_max are zero, so we go through else if (adj_chunk == 0) which calls the existing adjust_chunk_size_and_max_chunks free function, which is exactly the same code path as before.
Only an execution parameters object that provides a custom adjust_chunk_size_and_max_chunks member will return non-zero values and override the defaults.

[hkaiser]

I don,t think that the {0, 0} default changes the current behavior for any algorithm.

The call site in chunk_size.hpp

auto [adj_chunk, adj_max] =
    hpx::execution::experimental::adjust_chunk_size_and_max_chunks(
        policy.parameters(), policy.executor(),
        max_chunks, chunk_size);
if (adj_chunk != 0)
    chunk_size = adj_chunk;
if (adj_max != 0)
    max_chunks = adj_max;
else if (adj_chunk == 0)
    adjust_chunk_size_and_max_chunks(
        cores, count, max_chunks, chunk_size);

works when the Customization Point returns the default {0, 0}, both adj_chunk and adj_max are zero, so we go through else if (adj_chunk == 0) which calls the existing adjust_chunk_size_and_max_chunks free function, which is exactly the same code path as before. Only an execution parameters object that provides a custom adjust_chunk_size_and_max_chunks member will return non-zero values and override the defaults.

I have seen that. This code does something different from what was in place before. The default just should be the previous adjust_chunk_size_and_max_chunks (most likely to be invoked from the CPO fallback).

As said, please have a look at the other parameters customizations to see what's missing, e.g., get_chunk_size here: https://github.com/STEllAR-GROUP/hpx/blob/b514f4ab6274d21a716e3acaac08f825b9a93776/libs/core/execution/include/hpx/execution/executors/execution_parameters_fwd.hpp#L79-L127.

[hkaiser]

There is one thing missing still. Currently you use policy.with() to replace the current set of parameter customizations, which will cause for any user-supplied customizations to be lost. I think we need to use something like the new API rebind_executor_parameters (like for instance proposed here: #6935) instead.

Otherwise: very nice! Thanks for working on this!

[hkaiser]

There is one thing missing still. Currently you use policy.with() to replace the current set of parameter customizations, which will cause for any user-supplied customizations to be lost. I think we need to use something like the new API rebind_executor_parameters (like for instance proposed here: #6935) instead.

Otherwise: very nice! Thanks for working on this!

Let's wait for #6935 to be merged, then you can adapt your PR.

[hkaiser]

@BhoomishGupta FWIW, #6935 was merged yesterday. Could you please rebase this PR onto master to resolve the conflicts? After that let's work on finalizing your patch.

[BhoomishGupta]

@BhoomishGupta FWIW, #6935 was merged yesterday. Could you please rebase this PR onto master to resolve the conflicts? After that let's work on finalizing your patch.

Hi @hkaiser,

Thanks for the heads-up on #6935 getting merged!

Just a quick note: my university exams are currently going on, so my availability is a bit limited right now. I will rebase this PR onto master to resolve the conflicts and work on finalizing the patch right after my exams finish on March 14th.

Thanks for your patience!

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[hkaiser]

@BhoomishGupta On MacOS the new tests are failing. Inspect reports an issue as well. Please pay attention to the CIs yourself.

[BhoomishGupta]

@BhoomishGupta On MacOS the new tests are failing. Inspect reports an issue as well. Please pay attention to the CIs yourself.

Ah, my bad, I had my wires crossed and was looking at the CI for a different PR. I will check the MacOS and Inspect logs for this one tomorrow and will push a fix soon. I'll pay closer attention to the CI status here going forward.