Improvements to dash::accumulate

[devreal]

The current implementation of dash::accumulate has a flaw: only unit 0 (randomly chosen?) actually receives a valid result. This is neither documented nor does the user have a chance to influence this. I seem to remember that we had a similar discussion at our last F2F. IMO, either the user can choose which unit receives the result or it should be broadcast to all units. At the very least, it has to be documented...

Also, dash::accumulate should use dart_accumulate whenever possible.

Discovered while working on #212.

[fuchsto]

The current realization of dash::accumulate is somewhat irritating, yes. Oh yes.

First things first:

• dart_accumulate semantically corresponds to MPI_Accumulate: C[i] = reduce(A[i], B[i])
• dash::accumulate semantically corresponds to std::accumulate: Acc = reduce([AccInit] v A[s...e])

Consequently, dart_accumulate is not a semantically suitable backend for dash::accumulate.
C++ and MPI differ in their definitions of "accumulate" (C++ uses the term correctly, however std::transform should be named std::map, but can't, because of the container. It's a bloody mess).

Semantics of dart_accumulate correspond to dash::transform (~ std::transform), and it is in fact used as its communication backend:

https://github.com/dash-project/dash/blob/development/dash/include/dash/algorithm/Transform.h#L252

The best I could do for this clash of specs was to document it:

• https://github.com/dash-project/dash/blob/development/dash/include/dash/algorithm/Accumulate.h#L70
• https://github.com/dash-project/dash/blob/development/dash/include/dash/algorithm/Accumulate.h#L118

Standardese aside, the current implementation is broken.
I found another defect: the initial value is accumulated in every local partial result, but should only be accumulated once, at unit 0. Currently implemented behavior is:

// (a, b] = { 1,2,3, 4,5,6 }
// --> accumulate(a,b,+,10) = 31

unit 0:                          | unit 1:
=================================+==================================
dash::accumulate(a, b, +, 10);   | dash::accumulate(a, b, +, 10);
--> local result: 10 + 1 + 2 + 3 | --> local result: 10 + 4 + 5 + 6
    = 16                         |     = 25
----------------------------- barrier ------------------------------
result = 16 + 25 = 41 != 31

[devreal]

Thanks for the enlightenment! Then maybe accumulate_impl should not reside in Accumulate.h since the semantics are different? Who owns the code?

[fuchsto]

Emm. You moved it there? Or @fmoessbauer did and you reviewed it:
71d57b7

I implemented the algorithm stuff and placed accumulate_impl in Transform.h
... but we all own the code!

[devreal]

I know. Let me rephrase: Who will fix it? ;)

[fuchsto]

Oh :D
I will, my original implementation is broken anyways.

[fuchsto]

... aaaand also: as it is defined in DASH, accumulate_impl should be named transform_impl, because that's the DASH semantics.

[fuchsto]

Resolved in PR #237