RFC: Continuation of #20607, revision of sum, prod, and reduce #22825

TotalVerb · 2017-07-15T23:02:01Z

(Continuation of #20607 by @felixrehren. The intermediate commit should be preserved, but we should run the tests on it.)

This is not done yet. I'm creating this PR to solicit feedback about this approach.

Remaining things to do:

mr_empty should grow a specialization for compositions of promote_sys_size and something else (avoids regressions in empty case of sum(abs, ...)). This empty summation is apparently untested so tests will be needed to.
Review remaining methods of sum and prod to ensure things are consistent (and test)
Review and test that the dimensional reduction methods work properly
Document properly the new behavior
Doctests
Run test suite on the intermediate commit
Nanosoldier and make sure no perf regressions
NEWS entry

Certain design decisions that are in addition to the consensus discussed in #20560, #20607, and #21523, (I'm fairly convinced that they're the right thing to do, but please leave feedback if you disagree):

sum(Int8(1)) is now Int(1) instead of Int8(1). The previous behavior was inconsistent with other iterators. While it's true that this case cannot cause any problems, it's preferable in my opinion to always return a consistent result.
r_promote and related things are entirely gone. It's too much effort to properly deprecate them. It's not particularly useful anyway: promotion behavior can be passed in by the caller through v0 or f.
sum of integer ranges has always had this behavior by accident... by calling length() and using the result of that in the calculation. So I've left them alone.
I restricted the mr_empty fallback for & to apply only to the identity / Bool case.
Some miscellaneous overhaul of how mr_empty is defined to look more sane.

Certain things NOT addressed in this PR:

sum, prod of Tuples ignores the promotion behavior of arrays and other iterables. TODO items have been added.

bramtayl · 2017-07-16T13:29:37Z

mr_empty 'cause you can look right through me
Walk right by me and never know I'm there

stevengj · 2017-07-16T19:00:22Z

sum(Int8(1)) is now Int(1) instead of Int8(1).

I don't see any need for this. Nor is it inconsistent, in the sense of type-stability. Simpler to just keep sum(x::Number) = x?

stevengj · 2017-07-16T19:02:52Z

base/reduce.jl

+mr_empty(::typeof(identity), op::typeof(+), T) = zero(T)::T
+mr_empty(::typeof(abs), op::typeof(+), T) = abs(zero(T)::T)
+mr_empty(::typeof(abs2), op::typeof(+), T) = abs2(zero(T)::T)
+mr_empty(::typeof(identity), op::typeof(*), T) = one(T)::T


This is incorrect, since one(T) may not be of type T for a dimensionful type. Just get rid of the typeassert here.

Will fix, thanks. Actually it would be nice to see if any of these type annotations are really still necessary, since MethodError now infers as Union{} instead of Any. I will remove them all and we can see if any benchmarks regress.

All these annotations are gone now.

tkelman · 2017-07-18T08:03:37Z

base/reducedim.jl

@@ -574,14 +577,22 @@ any!(r, A)
 for (fname, op) in [(:sum, :+), (:prod, :*),
                    (:maximum, :scalarmax), (:minimum, :scalarmin),
                    (:all, :&), (:any, :|)]
+    function compose_pss(x)


only used once, abbreviating the name isn't that helpful

makes sense, wrote it in full

stevengj · 2017-07-19T16:27:38Z

@TotalVerb, I agree it's probably not worth a special case for sum(::Number).

StefanKarpinski · 2017-08-24T18:20:38Z

This got stalled out, but it would be great to make some progress here. Anything we can do to help?

TotalVerb · 2017-08-27T19:58:42Z

Going to pick this back up now and work through the remaining TODO items. Mostly, it should be just testing and confirming the consistency of the slice-based reductions.

TotalVerb · 2017-09-04T20:30:10Z

Another change to add to the unrelated changes list: I restricted the mr_empty fallback for & to apply only to the identity / Bool case.

TotalVerb · 2017-09-04T20:52:54Z

The code change is done. I still need to write up a NEWS entry and some doctests. In the meantime, could someone run nanosoldier on the latest revision?

cc @StefanKarpinski

KristofferC · 2017-09-04T21:06:04Z

base/reduce.jl

@@ -228,38 +228,41 @@ pairwise_blocksize(::typeof(abs2), ::typeof(+)) = 4096

 # handling empty arrays
 _empty_reduce_error() = throw(ArgumentError("reducing over an empty collection is not allowed"))
-mr_empty(f, op, T) = _empty_reduce_error()


RIP Mr. Empty :(

StefanKarpinski · 2017-09-06T03:29:59Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-09-06T04:02:40Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

TotalVerb · 2017-09-06T04:08:39Z

If this is the error, it looks unrelated:

ERROR: LoadError: MethodError: no method matching ind2sub(::Tuple{Int64,Int64}, ::CartesianIndex{2})
Closest candidates are:
  ind2sub(!Matched::AbstractArray, ::Any) at abstractarray.jl:1617
  ind2sub(::Tuple{Vararg{Integer,N}} where N, !Matched::Integer) at abstractarray.jl:1694
ind2sub(::Union{Tuple{Vararg{Integer,N}}, Tuple{Vararg{AbstractUnitRange,N}}}, !Matched::AbstractArray{#s51,1} where #s51<:Integer) where N at abstractarray.jl:1750

Please excuse me if this is unrelated, as I have not read nanosoldier logs before.

ararslan · 2017-09-06T04:13:22Z

Unrelated to this PR. Looks like BaseBenchmarks was broken by #22907.

TotalVerb · 2017-09-10T19:30:40Z

Could someone please run nanosoldier again?

KristofferC · 2017-09-10T19:32:29Z

@nanosoldier runbenchmarks(ALL, vs=":master")

ararslan · 2017-09-10T19:32:42Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-09-10T20:05:34Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

nanosoldier · 2017-09-10T20:05:41Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

TotalVerb · 2017-09-10T20:20:14Z

This failure looks related to the mapreduce changes. I'll investigate.

JuliaLang/julia#22825 will change the behavior of `mapreduce` to never promote, so to retain the previous behavior it must be changed to `sum`.

TotalVerb · 2017-09-12T20:46:05Z

Could someone please review and merge the associated BaseBenchmarks PR,
JuliaCI/BaseBenchmarks.jl#115

JuliaLang/julia#22825 will change the behavior of `mapreduce` to never promote, so to retain the previous behavior it must be changed to `sum`.

ararslan · 2017-09-12T20:57:19Z

I'm retuning the Nanosoldier benchmarks now that your change has been merged and I'll start a new Nanosoldier run here once that's done.

JeffBezanson · 2017-09-19T17:51:20Z

base/reduce.jl

+# reduced over to an appropriate size.
+promote_sys_size(x) = x
+promote_sys_size(x::SmallSigned) = Int(x)
+promote_sys_size(x::SmallUnsigned) = UInt(x)


This should also include Bool. I believe that will fix the problem in BaseBenchmarks.

Thanks, I'll fix and test this.

JeffBezanson · 2017-10-02T18:40:14Z

Bump. This is so close!

TotalVerb · 2017-10-06T19:48:38Z

Ok, here's a revised approach that I hope should handle Bool well. It passes test-reduce locally, so could someone try running nanosoldier again?

ararslan · 2017-10-06T19:51:28Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-10-06T23:45:42Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

JeffBezanson · 2017-10-15T17:22:21Z

Once this passes I'll squash all but the first commit and merge this.

JeffBezanson · 2017-10-15T19:00:50Z

Squashed version is on master; github might not pick up that this has been merged.

TotalVerb · 2017-10-15T19:03:36Z

Thanks!

StefanKarpinski · 2017-10-16T16:07:11Z

Thanks so much @TotalVerb for persisting with this change!

Use Compat.Test instead of Base.Test, adapt to JuliaLang/julia#22825

Since the demise of `r_promote` in #22825, there is now a type-instability in `mapreduce` if the operator does not give an element of the same type as the input. This arose during my implementation of Kahan summation using a reduction operator, see: JuliaMath/KahanSummation.jl#7 This adds a `mapreduce_single` function which defines what the result should be in these cases.

TotalVerb mentioned this pull request Jul 15, 2017

RFC reduce promotion: default to system default #20607

Closed

5 tasks

stevengj reviewed Jul 16, 2017

View reviewed changes

TotalVerb force-pushed the fw/wider branch from 1a265e4 to 6ff3631 Compare July 18, 2017 04:01

tkelman reviewed Jul 18, 2017

View reviewed changes

TotalVerb force-pushed the fw/wider branch from 6ff3631 to 8592dba Compare August 27, 2017 20:31

TotalVerb force-pushed the fw/wider branch from ea08a81 to c8d3ee7 Compare September 4, 2017 20:32

TotalVerb changed the title ~~WIP: Continuation of #20607, revision of sum, prod, and reduce~~ RFC: Continuation of #20607, revision of sum, prod, and reduce Sep 4, 2017

KristofferC reviewed Sep 4, 2017

View reviewed changes

TotalVerb added a commit to TotalVerb/BaseBenchmarks.jl that referenced this pull request Sep 10, 2017

Update sumindex.jl

acec604

JuliaLang/julia#22825 will change the behavior of `mapreduce` to never promote, so to retain the previous behavior it must be changed to `sum`.

TotalVerb mentioned this pull request Sep 10, 2017

Update sumindex.jl JuliaCI/BaseBenchmarks.jl#115

Merged

ararslan pushed a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Sep 12, 2017

Update sumindex.jl (#115)

a0c1b21

JuliaLang/julia#22825 will change the behavior of `mapreduce` to never promote, so to retain the previous behavior it must be changed to `sum`.

JeffBezanson reviewed Sep 19, 2017

View reviewed changes

TotalVerb mentioned this pull request Sep 21, 2017

make Bool <: Unsigned (i.e. UInt1) #19168

Closed

TotalVerb force-pushed the fw/wider branch from a9e45f3 to 55a9bda Compare October 6, 2017 19:47

felixrehren and others added 4 commits October 7, 2017 22:59

Default reduce promotion to system default

3c007af

Decouple sum/prod promotion from reduce

3272a40

Modifications to work well with Bool

1f587a4

Fix reducedim test

6347488

TotalVerb force-pushed the fw/wider branch from b28691d to 6347488 Compare October 10, 2017 03:13

Merge branch 'master' into fw/wider

3465d69

JeffBezanson closed this Oct 15, 2017

TotalVerb deleted the fw/wider branch October 15, 2017 19:03

tkoolen added a commit to tkoolen/TypeSortedCollections.jl that referenced this pull request Dec 4, 2017

Use Compat.Test instead of Base.Test, adapt to JuliaLang/julia#22825.

0d8a9cd

tkoolen added a commit to tkoolen/TypeSortedCollections.jl that referenced this pull request Dec 4, 2017

Merge pull request #16 from tkoolen/nightly

1c2bef2

Use Compat.Test instead of Base.Test, adapt to JuliaLang/julia#22825

simonbyrne mentioned this pull request Dec 12, 2017

WIP: leverage Base reduction functions JuliaMath/KahanSummation.jl#7

Closed

simonbyrne mentioned this pull request Dec 12, 2017

Add mapreduce_single function #25051

Merged

nalimilan mentioned this pull request Jan 25, 2018

mean() can easily overflow for general iterables #25739

Closed

NHDaly mentioned this pull request Jan 23, 2021

sum for tuples uses + #39182

Closed

petvana mentioned this pull request Jul 8, 2021

Fix sum() and prod() for tuples #41510

Merged

RFC: Continuation of #20607, revision of sum, prod, and reduce #22825

RFC: Continuation of #20607, revision of sum, prod, and reduce #22825

Conversation

TotalVerb commented Jul 15, 2017 • edited Loading

bramtayl commented Jul 16, 2017 • edited Loading

stevengj commented Jul 16, 2017 • edited Loading

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 Jul 19, 2017

StefanKarpinski commented Aug 24, 2017

TotalVerb commented Aug 27, 2017

TotalVerb commented Sep 4, 2017

TotalVerb commented Sep 4, 2017

Choose a reason for hiding this comment

StefanKarpinski commented Sep 6, 2017

nanosoldier commented Sep 6, 2017

TotalVerb commented Sep 6, 2017

ararslan commented Sep 6, 2017

TotalVerb commented Sep 10, 2017

KristofferC commented Sep 10, 2017

ararslan commented Sep 10, 2017

nanosoldier commented Sep 10, 2017

nanosoldier commented Sep 10, 2017

TotalVerb commented Sep 10, 2017

TotalVerb commented Sep 12, 2017

ararslan commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Oct 2, 2017

TotalVerb commented Oct 6, 2017

ararslan commented Oct 6, 2017

nanosoldier commented Oct 6, 2017

JeffBezanson commented Oct 15, 2017

JeffBezanson commented Oct 15, 2017

TotalVerb commented Oct 15, 2017

StefanKarpinski commented Oct 16, 2017

TotalVerb commented Jul 15, 2017 •

edited

Loading

bramtayl commented Jul 16, 2017 •

edited

Loading

stevengj commented Jul 16, 2017 •

edited

Loading