-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Implement accumulate
and friends for arbitrary iterators
#34656
Conversation
|
||
function iterate(itr::Accumulate) | ||
state = iterate(itr.itr) | ||
if state === nothing | ||
return nothing | ||
end | ||
return (state[1], state) | ||
val = Base.BottomRF(itr.f)(itr.init, state[1]) |
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.
Since Base.BottomRF(itr.f)
calls reduce_first
, this patch changes the output:
Before:
julia> collect(Iterators.accumulate(+, (x for x in [true])))
1-element Array{Bool,1}:
1
julia> collect(Iterators.accumulate(+, (x for x in [true, true, false])))
3-element Array{Integer,1}:
true
2
2
After:
julia> collect(Iterators.accumulate(+, (x for x in [true])))
1-element Array{Int64,1}:
1
julia> collect(Iterators.accumulate(+, (x for x in [true, true, false])))
3-element Array{Int64,1}:
1
2
2
Ref:
Lines 73 to 78 in 3720edf
struct BottomRF{T} | |
rf::T | |
end | |
@inline (op::BottomRF)(::_InitialValue, x) = reduce_first(op.rf, x) | |
@inline (op::BottomRF)(acc, x) = op.rf(acc, x) |
Bump. Can this be reviewed and merged? (The test failure in macOS looks like something irrelevant from |
@test cumsum(x^2 for x in 1:3) == [1, 5, 14] | ||
@test cumprod(x + 1 for x in 1:3) == [2, 6, 24] | ||
@test accumulate(+, (x^2 for x in 1:3); init=100) == [101, 105, 114] | ||
end |
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.
Maybe add a test for Iterators.accumulate
taking an init
keyword?
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.
Some more tests are added in 78a6c44
Nice addition! I was missing this functionality today. I will merge in a couple of days if no objections (please @tkf ping me if I forget). |
ping :) |
@@ -250,6 +262,10 @@ julia> accumulate(+, fill(1, 3, 3), dims=2) | |||
``` | |||
""" | |||
function accumulate(op, A; dims::Union{Nothing,Integer}=nothing, kw...) |
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.
Sorry that I missed that on first review: wouldn't this function need a compat annotation, like cumsum
and cumprod
?
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.
Thanks for catching it. I added it in 7a4d7fe.
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.
Thanks, in the meantime I also thought about compat for the iterator version, and was sorry to have to ask you again another change, but you already made it :)
As this changes very slightly the result of some accumulate operations (cf https://github.com/JuliaLang/julia/pull/34656/files#r374510231), I will apply the "minor change" label, but this migth be too minor to deserve it. |
This PR implements
Base.accumulate(op, itr)
simply by wrappingIterators.accumulate
. I addedinit
keyword argument toIterators.accumulate
to match the call signature.I'm posting this as a WIP PR as it is a patch on top #34654 which needs to go first.(Edit: ready for review now)