Fix and improve af::accum documentation #2298
Conversation
docs/details/algorithm.dox
Outdated
| \snippet test/scan.cpp ex_accum_1D | ||
|
|
||
| For 2D arrays (and higher dimensions), you can specify the dimension along which | ||
| the cumulative sum will be performed. If none is specified, it will be performed |
There was a problem hiding this comment.
I think computed/computed sounds better than performed.
There was a problem hiding this comment.
Yeah thanks, not sure why I wasn't consistent here, I did use "compute" in the brief description
docs/details/algorithm.dox
Outdated
|
|
||
| For 2D arrays (and higher dimensions), you can specify the dimension along which | ||
| the cumulative sum will be performed. If none is specified, it will be performed | ||
| along the first dimension (0): |
There was a problem hiding this comment.
Can you please verify if it dimension defaults to 0 or the first non-singleton dimension.
There was a problem hiding this comment.
Yeah sure, so I tried it:
array s = iota(dim4(1, 4));
af_print(s);
af_print(accum(s));s
[1 4 1 1]
Offset: 0
Strides: [1 1 4 4]
0.0000 1.0000 2.0000 3.0000
accum(s)
[1 4 1 1]
Offset: 0
Strides: [1 1 4 4]
0.0000 1.0000 2.0000 3.0000
Here dim1 is the first non-singleton dimension, and it looks like accum doesn't do anything to it. So I think it defaults to dim0. I should probably note though that this defaulting behavior is only on the C++ API.
There was a problem hiding this comment.
Sounds good.
PS. Defaults are only available for C++ API.
There was a problem hiding this comment.
Were you expecting the default to be -1 ? I was, but the code has 0 as default right now for whatever reason.
There was a problem hiding this comment.
I expect it to work on the first non-zero dimension. Does -1 do that? If it does then yes it should be the default. If it doesn't then it should be implemented.
There was a problem hiding this comment.
It should be fixed in a different PR
There was a problem hiding this comment.
-1 usually represents the first non-singleton dimension, this was the convention from the days of v2.x and we use it for most functions in v3.x also I believe.
include/af/algorithm.h
Outdated
|
|
||
| \param[in] in is the input array | ||
| \param[in] dim The dimension along which exclusive sum is performed | ||
| \return the output containing exclusive sums of the input | ||
| \param[in] dim is the dimension along which inclusive sum is performed |
There was a problem hiding this comment.
Similar to my first comment, perform is mostly with subjective contexts. Perhaps calculated or computed.
include/af/algorithm.h
Outdated
| \param[in] in is the input array | ||
| \param[in] dim The dimension along which exclusive sum is performed | ||
| \param[in] dim is the dimension along which inclusive sum is performed |
There was a problem hiding this comment.
Same as my previous comment about perform
|
@umar456 also requested that docs include a formula for the inclusive sum in the docs. Working on it. |
mark-poscablo
force-pushed
the
improve-accum-docs
branch
from
cdc4732 to
d69a4bb
Compare
|
Here's how it looks like now: |
docs/details/algorithm.dox
Outdated
|
|
||
| For 2D arrays (and higher dimensions), you can specify the dimension along which | ||
| the cumulative sum will be performed. If none is specified, it will be performed | ||
| along the first dimension (0): |
There was a problem hiding this comment.
It should be fixed in a different PR
The accum docs has some typos, such as saying "exclusive sum" on the function docs while it is really an inclusive sum scan. Also I think the brief description needs to say "cumulative" somewhere - "inclusive sum (scan)" didn't really ring a bell to me until I started learning parallel programming (a Google search on "inclusive sum" also doesn't immediately get clear results that it means inclusive sum scan). I added an example too just so it's clearer what it does.
Here are some snapshots of the docs after the change:
