Skip to content

Conversation

bernhardmgruber
Copy link
Contributor

@bernhardmgruber bernhardmgruber commented Mar 1, 2023

std::reduce can only be used over std::accumulate when the binary operation is commutative and associative. This does not hold for the exercises computing squared sums, so std::reduce is a bug there.

`std::reduce` can only be used over `std::accumulate` when the binary operation is commutative.
@bernhardmgruber bernhardmgruber added this to the March23 milestone Mar 1, 2023
@hageboeck
Copy link
Contributor

I'm puzzled by the motivation for the change. The op should be commutative, but it's likely not associative (i.e. different groupings lead to a different end result). I wouldn't say is wrong, though. It's not deterministic. Did I overlook something?

@hageboeck
Copy link
Contributor

BTW: The same would hold for the simple sum.

@bernhardmgruber
Copy link
Contributor Author

The op should be commutative, but it's likely not associative (i.e. different groupings lead to a different end result).

We are both right. The documenation says for the binary function:

The behavior is non-deterministic if binary_op is not associative or not commutative.

auto op = [](const T& s, const T& a) { return s + a * a; } is definitely neither commutative nor associative. op(1, 2) == 5 and op(2, 1) == 3. and op(1, op(2, 3)) == 122 and op(op(1, 2), 3) == 14.

I wouldn't say is wrong, though. It's not deterministic. Did I overlook something?

For computing the sum of squares, this is definitely wrong IMO. For the plain sum, your may get different rouding errors, which are fine IMO.

@hageboeck
Copy link
Contributor

hageboeck commented Mar 5, 2023 via email

@bernhardmgruber bernhardmgruber merged commit 10006cf into hsf-training:master Mar 5, 2023
@bernhardmgruber bernhardmgruber deleted the fix_reduce branch March 5, 2023 14:24
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.

2 participants