Skip to content
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

cumsum should accept functions and generic iterables #21150

Open
CarloLucibello opened this issue Mar 24, 2017 · 8 comments
Open

cumsum should accept functions and generic iterables #21150

CarloLucibello opened this issue Mar 24, 2017 · 8 comments
Labels
Hacktoberfest Good for Hacktoberfest participants help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@CarloLucibello
Copy link
Contributor

CarloLucibello commented Mar 24, 2017

I would find it useful and also more consistent with sum

julia> sum(i for i=1:10)
55

julia> cumsum(i for i=1:10)
ERROR: MethodError: no method matching cumsum(::Base.Generator{UnitRange{Int64},##11#12})
Closest candidates are:
  cumsum(::AbstractArray{T,N} where N) where T at multidimensional.jl:611
  cumsum(::AbstractArray{T,N} where N, ::Integer) where T at multidimensional.jl:611

julia> sum(x->x^2,1:10)
385

julia> cumsum(x->x^2,1:10)
ERROR: MethodError: no method matching cumsum(::##15#16, ::UnitRange{Int64})

julia> methods(cumsum)
# 2 methods for generic function "cumsum":
cumsum(A::AbstractArray{T,N} where N) where T in Base at multidimensional.jl:611
cumsum(A::AbstractArray{T,N} where N, axis::Integer) where T in Base at multidimensional.jl:611
@fredrikekre fredrikekre added Hacktoberfest Good for Hacktoberfest participants help wanted Indicates that a maintainer wants help on an issue or pull request labels Sep 28, 2017
@akshu3398
Copy link

akshu3398 commented Oct 14, 2020

It looks like cumsum(i for i=1:10) is fixed now. But, cumsum(x->x^2,1:10) still doesn't work. Even though it's solution is as easy as cumsum(f, x) = cumsum(f(i) for i ∈ x), it's surprising to see that no one has solved this issue yet.

I would like to work on it, if this issue is still open, and claim my 1st open-source contribution.

Also, if it's not much trouble, I'd like someone to guide me on how to implement this(what are the steps and correct ways to add this into julia) as this would be my 1st open-source contribution/pr.

@timholy
Copy link
Member

timholy commented Oct 14, 2020

Usually your best bet is to start from similar functions, for example @edit cumsum([1,2,3]) and @edit maximum(x->x^2, [1,2,3]). Also, use Revise.track(Base) for a more efficient workflow.

@abhinav3398
Copy link

abhinav3398 commented Oct 14, 2020

@timholy thank you for the tip to use Revise. I wrote the method(in Base) but, I meant, should I write some tests or docs before opening pr or leave that for another issue(1 feature/change at a time)?

P.S. ignore the user account change in the previous comment(this is the same user as @akshu3398). Again apologies for confusion in user.

@vtjnash
Copy link
Member

vtjnash commented Oct 14, 2020

Tests and docs are all part of the same change. Once you open a PR, people will help guide through any additional questions during review!

@DNF2
Copy link

DNF2 commented Oct 14, 2020

The idea of a mapaccumulate function came up in this discussion: https://discourse.julialang.org/t/broadcasting-and-function-composition-circ/46748/10

This is more general, and cumsum would then be mapaccumulate(f, +, x). Similarly, you would get cumprod, etc.

cumsum and cumprod are currently implemented in terms of accumulate, so it may be fitting, and symmetrical with mapreduce, as sum(f, x) is implemented as mapreduce(f, +, x).

@DNF2
Copy link

DNF2 commented Oct 14, 2020

cumsum(f, x) = cumsum(f(i) for i ∈ x)

This also does not accept the dim keyword. Piggybacking off something based on accumulate would help fixing pre-allocation of output containers and also supporting init.

@abhinav3398
Copy link

abhinav3398 commented Oct 14, 2020

cumsum and cumprod are currently implemented in terms of accumulate, so it may be fitting, and symmetrical with mapreduce, as sum(f, x) is implemented as mapreduce(f, +, x).

Hmm... @DNF2 you are probably right.
Looks like i'll have to look at #21152, #25766 & #34656 before i work on this.

abhinav3398 added a commit to abhinav3398/julia that referenced this issue Oct 15, 2020
@abhinav3398
Copy link

How about this Generator version? it should work for almost all of the iterators.

julia> cumsum(x->x^2, 1:2:10)
5-element Array{Int64,1}:
   1
  10
   
 165

julia> cumsum(x->x^2, 1:5)
5-element Array{Int64,1}:
  1
  5
  
 55

julia> cumsum(*, 1:3, 4:6, 7:9)
3-element Array{Int64,1}:
  28
 108
 270

julia> cumsum(*, [1,2,3], [4,5,6], [7,8,9])
3-element Array{Int64,1}:
  28
 108
 270

I know it's not using mapaccumulate, but since it's not done yet I thought why not do some temporary solution(hence, no pr, just for discussion). And this issue is specific to Iterables(and I assume Generators also), I chose to write specifically for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Good for Hacktoberfest participants help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

7 participants