use TypeArithmetic trait in cumsum! implementation #21666
Conversation
| @@ -574,12 +574,13 @@ function accumulate_pairwise(op, v::AbstractVector{T}) where T | |||
| end | |||
|
|
|||
| function cumsum!(out, v::AbstractVector, axis::Integer=1) | |||
| # for types prone to numerical stability issues, we want | |||
There was a problem hiding this comment.
This comment seems worth preserving in some form?
base/multidimensional.jl
Outdated
| end | ||
|
|
||
| function cumsum!(out, v::AbstractVector{<:Integer}, axis::Integer=1) | ||
| function _cumsum!(out, v, axis, ::ArithmeticRounds) | ||
| axis == 1 ? accumulate_pairwise!(+, out, v) : copy!(out,v) |
There was a problem hiding this comment.
Missing space between arguments to copy!, and likewise below?
|
What is the motivation for this? |
|
It was a hack. I wanted to dispatch on possibility of rounding errors, when I did #18931. But that trait did not yet exist, so I dispatched on |
base/multidimensional.jl
Outdated
| function _cumsum!(out, v, axis, ::ArithmeticRounds) | ||
| axis == 1 ? accumulate_pairwise!(+, out, v) : copy!(out, v) | ||
| end | ||
| function _cumsum(out, v, axis, ::ArithmeticUnknown) |
There was a problem hiding this comment.
Missing !? If so, perhaps add a test covering this code path?
|
should be squashed on merge, if not before |
|
@andreasnoack, lgty? :) |
|
A second approving review of this pull request would be great. But absent that, and absent objections, requests for time, or someone beating me to it, I plan to merge this tomorrow morning. Best! |
|
Thanks @timholy! :) |
very minor refactoring.