Incorrect reduce implementation

[K-ballo]

https://github.com/STEllAR-GROUP/hpx/blob/0005b929c69fd199c11dfd691f4d1b62d90735af/libs/core/algorithms/include/hpx/parallel/algorithms/reduce.hpp#L421

This line in reduce implementation attempts to convert *first to T, but there is no requirement for that operation. See https://eel.is/c++draft/reduce#5

As a somewhat artificial example, it's possible to implement minmax on top of reduce:

struct minmax
{
    std::pair<int, int> operator()(
        std::pair<int, int> lhs, std::pair<int, int> rhs) const
    {
        return {
            lhs.first < rhs.first ? lhs.first : rhs.first,
            lhs.second < rhs.second ? rhs.second : lhs.second,
        };
    }
    std::pair<int, int> operator()(std::pair<int, int> lhs, int rhs) const
    {
        return (*this)(lhs, std::pair<int, int>{rhs, rhs});
    }
    std::pair<int, int> operator()(int lhs, std::pair<int, int> rhs) const
    {
        return (*this)(std::pair<int, int>{lhs, lhs}, rhs);
    }
    std::pair<int, int> operator()(int lhs, int rhs) const
    {
        return (*this)(std::pair<int, int>{lhs, lhs}, std::pair<int, int>{rhs, rhs});
    }
};

auto [min, max] = hpx::reduce(hpx::execution::par,
    c.begin(), c.end(), std::pair<int, int>{INT_MAX, INT_MIN}, minmax{});

[kairveeehh]

hii @K-ballo I would like to work on this issue , my proposed changes would be to change it to

T val = init;
for (auto it = part_begin; it != part_end; ++it)
{
    val = HPX_INVOKE(binary_op, val, *it);
}

to ensure the correct usage. could you guide me if I am thinking in the correct direction and should proceed?

[hkaiser]

I would like to work on this issue

Please feel free.

[kairveeehh]

I would like to work on this issue

Please feel free.

could you confirm me if i am thinking in the right direction?

[hkaiser]

I would like to work on this issue

Please feel free.

could you confirm me if i am thinking in the right direction?

Yes, if the use case shown above works as expected after applying the change you suggested.

[sleepingeight]

Hello @hkaiser
I would like to point out an issue in migrating the current reduce implementation according to the standard:

We would like to have a parallel version of reduce, in which the task is distributed among multiple threads. Each thread needs to start with an initial value which needs to be the identity of given binary operator. Currently we tweaked this logic by using T val = *part_begin;. But this needs to be replaced with an identity such as 0 for std::plus. As the binary operations can be custom made, such as minmax (whose identity is {INT_MAX, INT_MIN}), it is only feasible to implement the parallel version of reduce only if the identity of the binary operation is provided by the caller himself.

I can implement this functionality by overloading, but wanted your opinion on this approach.