-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Deprecate cumsum, cumprod, cumsum_kbn, and accumulate when dim isn't specified #24684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Rebase? |
base/multidimensional.jl
Outdated
|
|
||
| """ | ||
| cumsum(A, dim=1) | ||
| cumsum(A, dim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT there should be a separate docstring for AbstractVector, right? Also, ::Integer would be useful since there's no longer any indication of the expected type in the signature.
e8d02ea to
5ff1bad
Compare
base/multidimensional.jl
Outdated
|
|
||
| """ | ||
| cumsum(A, dim) | ||
| cumsum(A, axis::Integer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep dim or use region, which are the two terms that appear to be common in similar functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what the method uses. They used to be out of sync. However, since we have reducedim, it is probably better to use dim here as well. I'll update.
base/multidimensional.jl
Outdated
| accumulate!(op, B, A, dim::Integer) | ||
| Cumulative operation `op` on `A` along the axis `axis`, storing the result in `B`. | ||
| Cumulative operation `op` on `A` along the dimensions `dim`, storing the result in `B`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Singular "dimension".
473479a to
92dfc4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the singular/plural issue with "dimensions" in several places.
Should be fixed now. |
specified and dimension is larger than one
Fix bug in accumulate when initial value is provided since the implementation doesn't support multidimentional arrays. Move the accumulate methods with initial values to the end
92dfc4d to
128f7c9
Compare
...and dimension is larger than one
Fixes #19451