Added accumulate, accumulate! #18931

jw3126 · 2016-10-14T18:33:53Z

Added accumulate, accumulate! see #14730.

StefanKarpinski · 2016-10-14T20:44:00Z

If anything, I feel like we should deprecate cumin and cumax in favor of scan(min, ...) and scan(max, ...) now that the latter has no performance down side. cumsum and cumprod are a bit different since sum and prod don't do exactly what a plain reduce would do and it may make sense for cumsum and cumprod to mirror what their non-cumulative counterparts do.

jw3126 · 2016-10-14T20:59:24Z

I would keep cummin and cummax. I find that scan is a terribly unintuitive name (albeit standard) and think cummin and cummax are easier to discover.

What do you mean, when you say that sum and prod are not exactly a reduce?

nalimilan · 2016-10-14T21:09:42Z

Indeed, where does the name choice come from? I did a bit a googling, and I couldn't easily find another example of scan used for cumulative operations. In many statistical packages, scan is used to parse a string.

StefanKarpinski · 2016-10-14T21:44:32Z

It's the standard name for the operation: https://en.wikipedia.org/wiki/Prefix_sum.

StefanKarpinski · 2016-10-14T21:44:59Z

Although I agree that it's a bit unfortunate. Perhaps we can pick a better name?

tkelman · 2016-10-14T21:47:23Z

base/deprecated.jl

@@ -144,6 +144,8 @@ end
 @deprecate chol(A::Number, ::Type{Val{:L}})         ctranspose(chol(A))
 @deprecate chol(A::AbstractMatrix, ::Type{Val{:L}}) ctranspose(chol(A))

+@deprecate cumop! scan!


cumop! wasn't exported, was it?

Right, is the policy that only exported functions get deprecated? Should I remove this line?

worth checking whether it's widely used in packages. if not, then maybe we can skip the deprecation. mainly the issue is the @deprecate macro exports the old deprecated name, so isn't quite the right tool for deprecating something that was not originally exported

nalimilan · 2016-10-14T21:56:06Z

Thanks for the link, it's funny it didn't show up. Personally, anything sounding like "cumulative" would be clearer for me, including cumop. But maybe we should have a look at what other languages do.

StefanKarpinski · 2016-10-14T22:52:58Z

I would be happy with cumulative and cumulative!. It's crystal clear.

ararslan · 2016-10-15T00:51:37Z

💯 for cumulative. When I first read the PR I thought scan was supposed to be like R's scan, which gets input from STDIN. It's a pretty unintuitive name, and I think cumulative is the perfect replacement.

Edit: accumulate (see below) is even better

Sacha0 · 2016-10-15T01:22:18Z

accumulate (verb) might be more consistent with the names of similar higher-order functions (e.g. reduce)?

jw3126 · 2016-10-15T06:14:46Z

Using a verb is a great idea. I think I will go with cumulate, which is slightly shorter then accumulate and closer to the word cumulative.

ararslan · 2016-10-15T06:29:32Z

I actually didn't know that "cumulate" was a word until I looked it up just now. It might be nice to stick with a more common word, at least IMHO.

KristofferC · 2016-10-15T09:32:07Z

As another data point. For me, a non native speaker, accumulate is a lot clearer.

jw3126 · 2016-10-15T10:23:41Z

I must confess that I was also not 100% sure whether cumulate was actually a word and looked it up to be safe 😄 . So maybe accumulate is indeed better.

Sacha0 · 2016-10-15T16:00:18Z

:)

KristofferC · 2016-10-15T16:04:34Z

If it was 1680 we might have had to bike shed this for a while :P

timholy · 2016-10-15T19:35:14Z

I really like accumulate.

Speaking of renaming and functions that carry out this type of task, do we want to rename the internal function r_promote as something a little more...memorable? reusable? How about accumulate_eltype? I'm not sure we need to export it, but the choice of an element type for reductions/accumulations is a pretty common and fundamental task. I would be surprised if there aren't at least a half dozen semi-incompatible implementations of similar logic in packages. Perhaps it makes sense to have one in Base that can be exploited.

jw3126 · 2016-10-15T21:36:20Z

I wonder should we have a left and a right accumulate_eltype?

StefanKarpinski · 2016-10-16T17:21:27Z

+1 on accumulate – it's better than cumulative.

tkelman · 2016-10-17T12:58:46Z

base/multidimensional.jl

+"""
+    accumulate!(op, B, A, dim=1)
+
+Cumulative operation `op` on A along a dimension, storing the result in B. The dimension defaults to 1.


do we usually code highlight the input names A, B etc ?

edit: looks like the cumprod! docstring does

jw3126 · 2016-10-21T20:40:11Z

@timholy Turns out that there are already two such semi incompatible implementations in Base. Namely r_promote and rcum_promote_type. The issue is, that widening of numeric types gives precision, but costs memory. Now in case of reductions it does not matter, whether your result is 2, 4 or 8 bytes of memory, so small numeric types will be widened.
But in the case of accumulate this can make a difference and it was argued in #18364 that you probably don't want to widen automatically in that case.

I would still tend to merge r_promote and rcum_promote_type into a single accumulate_eltype, which does widening like r_promote. This way behaviour of reduce and accumulate would be more coherent. If you don't want widening however would have to use more verbose things like
cumsum!(similar(A), A)
cc @stevengj @tkelman

tkelman · 2016-10-21T22:05:25Z

We don't widen Float32 to Float64 in reductions, do we? We widen integer types which gives more range but not more precision. I think for an operation that returns an array, widening should be something that the user has to ask for.

jw3126 · 2016-10-22T07:48:43Z

Right it is more about overflow, though Float16 is also widened. If reduce and accumulate behave differently I guess it makes not sense to have accumulate_eltype?

timholy · 2016-10-22T17:08:15Z

Good point, it's more an issue for reductions than for accumulations---manual specification makes sense for the latter.

tkelman · 2016-10-24T19:03:28Z

base/multidimensional.jl

-        ($fmod)(res, A, R1, R2, axis)
-    end
+function accumulate_pairwise!(op, result::AbstractVector, v::AbstractVector)
+


remove blank line here

ararslan · 2016-10-24T19:34:37Z

test/arrayops.jl

@@ -1905,4 +1908,8 @@ end
        end
    end

+    # exotic intexing


intexing -> indexing

KristofferC · 2016-10-24T19:46:59Z

base/multidimensional.jl

+    return s_
+end
+
+function accumulate_pairwise!(op, result::AbstractVector, v::AbstractVector)


Does this need function accumulate_pairwise!{F}(op::F, result::AbstractVector, v::AbstractVector) Ref: #18207

Good catch! I will test if there is a performance difference. Since each final call covers 128 elements of v, it might be okay.

Did you get any results? Same should apply to the function below?

I had only one allocation even without op::F, not sure why? I added the type annotation to _accumulate_pairwise! anyway however.

stevengj · 2016-10-25T17:34:37Z

NumPy also calls a similar operation accumulate, so +1 for that name.

Regarding widening, my inclination is also to not automatically widen. The only corner case is for cumsum of Bool, which we widen to Int, and we opted to be conservative and preserve this behavior in #18364.

yuyichao · 2016-11-18T18:51:23Z

test/arrayops.jl

@@ -1868,3 +1868,61 @@ end
        @test size(a) == size(b)
    end
 end
+
+isdefined(:TestHelpers) || eval(Main, :(include(joinpath(dirname(@__FILE__), "TestHelpers.jl"))))


As noted, the isdefined check is wrong.

jw3126 · 2016-11-19T20:05:50Z

done

Fixed

stevengj · 2016-11-21T13:42:01Z

~~Looks good to merge.~~ Actually, there should probably be a NEWS item for the accumulate function first.

StefanKarpinski · 2016-11-21T16:45:08Z

The last hurdle. Then we dine in... er, I mean then we merge this PR!

stevengj · 2016-11-21T18:03:48Z

NEWS.md

@@ -76,6 +78,7 @@ Deprecated or removed

  * `Dates.recur` has been deprecated in favor of `filter` ([#19288])

+  * `cummin` and `cummax` have been deprecated in favor of `accumulate`.
 Julia v0.5.0 Release Notes


missing a blank line

stevengj · 2016-11-21T18:04:39Z

NEWS.md

@@ -60,6 +60,8 @@ Library improvements
    you can now do e.g. `[A I]` and it will concatenate an appropriately sized
    identity matrix ([#19305]).

+  * New `accumulate` and `accumulate!` functions, which generalize `cumsum` and `cumprod`. Also known as [scan](https://en.wikipedia.org/wiki/Prefix_sum) ([#18931]).


, also known as a scan operation

stevengj · 2016-11-21T18:21:00Z

git rebase --whitespace=fix HEAD~1 to fix the trailing-whitespace appveyor failure.

jw3126 · 2016-11-23T19:37:30Z

Anything left that prevents this PR from merge?

tkelman · 2016-11-23T21:18:00Z

base/multidimensional.jl

+rcum_promote_type{T,N}(op, ::Type{Array{T,N}}) = Array{rcum_promote_type(op,T), N}
+
+# accumulate_pairwise slightly slower then accumulate, but more numerically
+# stable in certain situtations (e.g. sums).


typo here "situtations" has one too many t's

tkelman · 2016-11-25T08:01:27Z

base/deprecated.jl

@@ -145,6 +145,10 @@ end
 @deprecate chol(A::Number, ::Type{Val{:L}})         ctranspose(chol(A))
 @deprecate chol(A::AbstractMatrix, ::Type{Val{:L}}) ctranspose(chol(A))

+@deprecate cummin(A, dim=1) accumulate(min, A, dim=1)
+@deprecate cummax(A, dim=1) accumulate(max, A, dim=1)


these need to be at the end of the file where 0.6 deprecations go, not in the middle of the 0.5 deprecations

edit: got it in #19415

…ng#18931).

#21987)

#21987) (cherry picked from commit d74dbd7)

coveralls · 2019-01-28T07:54:51Z

Coverage increased (+26.1%) to 73.121% when pulling 08fcdf0 on jw3126:scan into 7232a9a on JuliaLang:master.

tkelman reviewed Oct 14, 2016

View reviewed changes

jw3126 changed the title ~~Added scan, scan!~~ Added accumulate, accumulate! Oct 16, 2016

jw3126 force-pushed the scan branch from bb91c00 to 11f03f1 Compare October 16, 2016 08:57

tkelman reviewed Oct 17, 2016

View reviewed changes

tkelman reviewed Oct 24, 2016

View reviewed changes

ararslan reviewed Oct 24, 2016

View reviewed changes

test/arrayops.jl

@@ -1905,4 +1908,8 @@ end

end

end

# exotic intexing

Copy link

Member

ararslan Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intexing -> indexing

KristofferC reviewed Oct 24, 2016

View reviewed changes

yuyichao previously requested changes Nov 18, 2016

View reviewed changes

fix

c50f459

stevengj removed the potential benchmark Could make a good benchmark in BaseBenchmarks label Nov 21, 2016

fix

641b539

stevengj reviewed Nov 21, 2016

View reviewed changes

Update NEWS.md

9ab7dc8

stevengj reviewed Nov 21, 2016

View reviewed changes

jw3126 force-pushed the scan branch from a866780 to ee64192 Compare November 21, 2016 18:25

Update NEWS.md

0b1d097

jw3126 force-pushed the scan branch from ee64192 to 0b1d097 Compare November 21, 2016 18:49

fix

08fcdf0

StefanKarpinski approved these changes Nov 23, 2016

View reviewed changes

StefanKarpinski merged commit 3daa183 into JuliaLang:master Nov 23, 2016

tkelman reviewed Nov 23, 2016

View reviewed changes

tkelman reviewed Nov 25, 2016

View reviewed changes

kmsquire mentioned this pull request Dec 16, 2016

scan! and scan functions #14730

Closed

fcard pushed a commit to fcard/julia that referenced this pull request Feb 28, 2017

Add accumulate, accumulate! (JuliaLang#18931)

e4f5e15

jw3126 mentioned this pull request May 2, 2017

use TypeArithmetic trait in cumsum! implementation #21666

Merged

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 20, 2017

Add PR number to NEWS.md entry for cummin/cummax deprecation (JuliaLa…

aa2c56f

…ng#18931).

ararslan pushed a commit that referenced this pull request May 24, 2017

Add PR number to NEWS.md entry for cummin/cummax deprecation (#18931). (

d74dbd7

#21987)

tkelman pushed a commit that referenced this pull request Jun 3, 2017

Add PR number to NEWS.md entry for cummin/cummax deprecation (#18931). (

f9fb392

#21987) (cherry picked from commit d74dbd7)

jw3126 mentioned this pull request Jan 16, 2018

fix non numeric accumulate(op, v0, x) (#25506) #25515

Closed

s-celles mentioned this pull request Jun 22, 2019

Different API with rolling function JeffreySarnoff/RollingFunctions.jl#14

Closed

Added accumulate, accumulate! #18931

Added accumulate, accumulate! #18931

Conversation

jw3126 commented Oct 14, 2016 • edited Loading

StefanKarpinski commented Oct 14, 2016

jw3126 commented Oct 14, 2016

nalimilan commented Oct 14, 2016

StefanKarpinski commented Oct 14, 2016

StefanKarpinski commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Oct 14, 2016

StefanKarpinski commented Oct 14, 2016

ararslan commented Oct 15, 2016 • edited Loading

Sacha0 commented Oct 15, 2016

jw3126 commented Oct 15, 2016

ararslan commented Oct 15, 2016

KristofferC commented Oct 15, 2016

jw3126 commented Oct 15, 2016

Sacha0 commented Oct 15, 2016

KristofferC commented Oct 15, 2016 • edited Loading

timholy commented Oct 15, 2016

jw3126 commented Oct 15, 2016

StefanKarpinski commented Oct 16, 2016

tkelman Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

jw3126 commented Oct 21, 2016 • edited Loading

tkelman commented Oct 21, 2016

jw3126 commented Oct 22, 2016

timholy commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Oct 25, 2016

Choose a reason for hiding this comment

jw3126 commented Nov 19, 2016

stevengj commented Nov 21, 2016 • edited Loading

StefanKarpinski commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Nov 21, 2016

jw3126 commented Nov 23, 2016

Choose a reason for hiding this comment

tkelman Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 28, 2019

jw3126 commented Oct 14, 2016 •

edited

Loading

ararslan commented Oct 15, 2016 •

edited

Loading

KristofferC commented Oct 15, 2016 •

edited

Loading

tkelman Oct 17, 2016 •

edited

Loading

jw3126 commented Oct 21, 2016 •

edited

Loading

stevengj commented Nov 21, 2016 •

edited

Loading

tkelman Nov 25, 2016 •

edited

Loading