#sum:, #detectSum:, #sumNumbers:

On 04 Dec 2015, at 01:49, Ben Coman <[hidden email]> wrote:

On Fri, Dec 4, 2015 at 4:23 AM, Nicolai Hess <[hidden email]> wrote:

2015-12-03 14:48 GMT+01:00 Ben Coman <[hidden email]>:

On Wed, Dec 2, 2015 at 10:45 PM, Sven Van Caekenberghe <[hidden email]>
wrote:

On 02 Dec 2015, at 15:21, Nicolai Hess <[hidden email]> wrote:



2015-12-02 15:03 GMT+01:00 Ben Coman <[hidden email]>:
On Wed, Dec 2, 2015 at 12:38 AM, Tudor Girba <[hidden email]>
wrote:

Hi,

On Dec 1, 2015, at 5:13 PM, Max Leske <[hidden email]> wrote:

@Doru
You’re missing the point: #anyOne *fails* for empty collections.

I am not missing the point at all. I am saying that if you want sum:
to be generic, it cannot assume a specific Zero object.

And sum: should be generic because of its name.

I am missing understanding the other use cases.  Can you describe
further the generic nature of #sum & #sum: ?  I would have thought by
default they only applied to numbers.

sum can be applied to anything that supports #+, not only numbers
sum: can be applied to any collection with a block that return some
object that supports #+

To me this is a mis-application of polymorphism, that just because
something responds to #+ it should be summable. We have overloaded the
semantics of  #+  to mean both numeric addition and
concatenation/membership, but technically "summation" relates only to
numeric addition.

I didn't wanted to argue for or against any change. I just wanted to clarify
that there are situations in which a generic sum/sum: that throws an error
on empty collections and don't assume a null value makes sense.

https://www.google.com.au/search?q=define+sum&oq=define+sum

https://www.google.com.au/search?q=define+concatenate&oq=define+concatenate

For example...

* KMModifier implements  #+  so what is the expected semantic of   " {
KMModifier shift . KMModifier meta} sum " ?
To me this is more of a concatenation/join/union facility rather than
numeric addition.  (btw, that expression actually fails since
KMModifier does not understand minus #- ) .

* String implements  #+  and  " { '1' . '2' } sum " --> '3',   so
actually its doing numeric addition.  However  " { 'a' . 'b' } sum "
produces an error.

So actually there seem some existing problems with summing
non-numerics.  What examples work?

* Trait classes implement both  #+  and  #- , but the semantic seems
more to do with membership than numeric addition.  I don't how how to
produce an example of using #sum against traits.

* Points are summable  " { 2@2 . 3@3 } " --> 5@5.   But then  " 2@2 +
1 " --> 3@3   ,   so  " {} sum "  returning  0  would seem to not
cause any error in this case.


cheers -ben

therefore you can not assume 0 (<- a number) is a proper initial value
therefore you *need* to work with #anyOne
and as you can not assume a proper initial value, you can not assume a
default value for empty collections
-> it should throw an error. If you (the caller of the function) knows
what to do with an empty collection you have
to check, or call inject:into: directly, with a proper initial value.

I am sorry but I am getting really tired of this, you should read what
is being said.

do that change, I am not against it. Ben just asked for an example and I
thought it would be helpful.

It was helpful :)  It evolved my thinking.   Now thinking further, I
wonder how returning  0  will work with applications using units like
Aconcagua, and if it would over-complicate things to do something
like...

   Collection>>sum
      | sum sample |
      self isEmpty ifTrue: [ ^ ArithmeticZero ].
      sample := self anyOne.
      sum := self inject: sample into: [ :accum :each | accum + each ].
       ^ sum - sample

   ArithmeticZero class >> + anObject
       ^anObject

cheers -ben

I am not suggesting to stop using #anyOne because I like why it is there
and what it can do.

The change I want is what happens with an empty collection when using
the simplest selector, #sum.

I do not want to say to some collection of numbers #sumIfEmpty: [0],
because summing numbers starting from zero is the most common case and
everybody expects that, hence the unary selector fits.

I want the less common cases to use the more complicated API, as in some
collection of colors #sumIfEmpty: [ Color black ]

In my book that is common sense API design.

Doing that I do no take anything away, because today you already have to
make sure the collection is not empty.

The only change would be in the error behaviour. I think that is a
reasonable price to pay. Instead of having #anyOne fail, it will say that #+
cannot add 0 to some object, and in a comment we can point to the
alternative API.

http://izquotes.com/quote/242740 right ?

cheers -ben

On 01 Dec 2015, at 15:31, Esteban A. Maringolo
<[hidden email]> wrote:

I don't want to be heretic (or too orthodox), but why not to
delegate
this behavior to other class (an iterator maybe?).

It's too tempting adding these convenience methods to Collection
and/or subclasses, but anything that requires an explicit protocol
of
its elements is wrong, IMO.

something like aCollection arithmetic sum: [...] or.... aCollection
arithmetic avg.

On Fri, Dec 4, 2015 at 3:26 AM, Chris Cunningham
<[hidden email]> wrote:

<uncontrolled snipping>

On Thu, Dec 3, 2015 at 5:48 AM, Ben Coman <[hidden email]> wrote:

* Points are summable " { 2@2 . 3@3 } " --> 5@5. But then " 2@2 +
1 " --> 3@3 , so " {} sum " returning 0 would seem to not
cause any error in this case.

but points aren't commutative:

2@2 + 1 " = 3@3"
1 + 2@2 " = 3@2"

Of course, 0 wouldn't be an issue, unless you wanted to access x or y!

On 05 Dec 2015, at 18:14, stepharo <[hidden email]> wrote:

So what is the conclusion?
I like the idea of Esteban M to have iterator because it moves some behavior out of core classes.

[[[

aCollection arithmetic sum: [...] or.... aCollection
arithmetic avg.

]]]

On 20 Dec 2015, at 13:43, Gabriel Cotelli <[hidden email]> wrote:

Max,

sum: aBlock ifEmpty: emptyBlock needs to obtain the sample evaluating the block.

sum: aBlock ifEmpty: emptyBlock

| sum sample |

self isEmpty ifTrue: [ ^ emptyBlock value ].

sample := aBlock value: self anyOne.

sum := self

inject: sample

into: [ :accum :each | accum + (aBlock value: each) ].

^ sum - sample

On Sun, Dec 20, 2015 at 8:59 AM, Max Leske <[hidden email]> wrote:

I would like to wrap up this discussion. 

On 05 Dec 2015, at 18:14, stepharo <[hidden email]> wrote:

So what is the conclusion?
I like the idea of Esteban M to have iterator because it moves some behavior out of core classes.

[[[

aCollection arithmetic sum: [...] or.... aCollection
arithmetic avg.

]]]

While I think that iterators are an intriguing idea I also believe that they are beyond the scope of this issue. If anybody wants to follow up on iterators (or unit types for that matter) please start a new thread / issue.

I propose to use Sven’s version for #sum:ifEmpty:. The result would be these three methods:

sum

^ self

sum: [ :each | each ]

ifEmpty: [ 0 ]

sum: aBlock

^ self

sum: aBlock

ifEmpty: [ self errorEmptyCollection ]

sum: aBlock ifEmpty: emptyBlock

| sum sample |

self isEmpty ifTrue: [ ^ emptyBlock value ].

sample := self anyOne.

sum := self

inject: sample

into: [ :accum :each | accum + (aBlock value: each) ].

^ sum - sample

I’ve attached a couple of benchmark results below. To me they show that

1. the new implementation is maybe a tiny bit slower but insignificantly so (if you’re going for performance you’ll probably write your own optimised version anyway)

2. there is no need to duplicate the code (like #sum and #sum: currently do). It’s perfectly fine to delegate to #sum:ifEmpty:

In addition to the above changes I would like to remove #detectSum: (-> #sum:) and #sumNumbers (-> #sum).

Note that once we agree on changing this API, we will need to also change #detectMin:, #detectMax:, #min, #max as well as all overrides (e.g. RunArray, Interval) of these and of #sum et. al. to stay consistent. The changes would of course be in line with this change, such that every operation has a unary selector with a sensible default, one that takes a block and throws an error for empty collections and a third that takes a block for the iteration and one for the empty case.

Please cast your vote for these changes:

1. Do you agree to changing #sum and #sum: in the suggested way?

2. Do you agree to the removal of #detectSum:?

3. Do you agree to the removal of #sumNumbers?

4. Do you agree that the #max and #min selectors also need to be adapted?

Thanks for you help.

Cheers,

Max

Benchmarks

============

(Note that these aren’t very good benchmarks. There’s quite some variation on each run.)

Machine:

MacBook Pro (15-inch, Early 2011)

CPU: 2.2 GHz Intel Core i7

Memory: 8 GB 1333 MHz DDR3

Disk: APPLE SSD TS512C (500 GB)

Benchmarks of the current versions:

[ (1 to: 1000000) asArray sum ] benchFor: 10 seconds.

75 iterations, 7.470 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ] benchFor: 10 seconds.

72 iterations, 7.128 per second

[ (1 to: 100) asArray sum ] benchFor: 10 seconds.

1,189,477 iterations, 118,912 per second

[ (1 to: 100) asArray sum: [ :e | e ] ] benchFor: 10 seconds.

1,171,467 iterations, 117,112 per second

Benchmarks of the new versions:

[ (1 to: 1000000) asArray sum ] benchFor: 10 seconds.

73 iterations, 7.244 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ] benchFor: 10 seconds.

75 iterations, 7.480 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ifEmpty: [ 0 ] ] benchFor: 10 seconds.

72 iterations, 7.141 per second

[ (1 to: 100) asArray sum ] benchFor: 10 seconds.

1,115,827 iterations, 111,560 per second

[ (1 to: 100) asArray sum: [ :e | e ] ] benchFor: 10 seconds.

1,154,595 iterations, 115,425 per second

[ (1 to: 100) asArray sum: [ :e | e ] ifEmpty: [ 0 ] ] benchFor: 10 seconds.

1,102,358 iterations, 110,203 per second

On 20 Dec 2015, at 14:01, Sven Van Caekenberghe <[hidden email]> wrote:

On 20 Dec 2015, at 12:59, Max Leske <[hidden email]> wrote:

I would like to wrap up this discussion. 

Good idea.

On 05 Dec 2015, at 18:14, stepharo <[hidden email]> wrote:

So what is the conclusion?
I like the idea of Esteban M to have iterator because it moves some behavior out of core classes.

[[[

aCollection arithmetic sum: [...] or.... aCollection
arithmetic avg.

]]]

While I think that iterators are an intriguing idea I also believe that they are beyond the scope of this issue. If anybody wants to follow up on iterators (or unit types for that matter) please start a new thread / issue.


I propose to use Sven’s version for #sum:ifEmpty:. The result would be these three methods:

sum
^ self
sum: [ :each | each ]
ifEmpty: [ 0 ]

sum: aBlock
^ self
sum: aBlock
ifEmpty: [ self errorEmptyCollection ]

sum: aBlock ifEmpty: emptyBlock
| sum sample |
self isEmpty ifTrue: [ ^ emptyBlock value ].
sample := self anyOne.
sum := self
inject: sample
into: [ :accum :each | accum + (aBlock value: each) ].
^ sum - sample


I’ve attached a couple of benchmark results below. To me they show that
1. the new implementation is maybe a tiny bit slower but insignificantly so (if you’re going for performance you’ll probably write your own optimised version anyway)
2. there is no need to duplicate the code (like #sum and #sum: currently do). It’s perfectly fine to delegate to #sum:ifEmpty:



In addition to the above changes I would like to remove #detectSum: (-> #sum:) and #sumNumbers (-> #sum).


Note that once we agree on changing this API, we will need to also change #detectMin:, #detectMax:, #min, #max as well as all overrides (e.g. RunArray, Interval) of these and of #sum et. al. to stay consistent. The changes would of course be in line with this change, such that every operation has a unary selector with a sensible default, one that takes a block and throws an error for empty collections and a third that takes a block for the iteration and one for the empty case.

Excellent summary, thanks.

Please cast your vote for these changes:

1. Do you agree to changing #sum and #sum: in the suggested way?

yes

2. Do you agree to the removal of #detectSum:?

yes

3. Do you agree to the removal of #sumNumbers?

yes

4. Do you agree that the #max and #min selectors also need to be adapted?

probably yes.

Thanks for you help.

Cheers,
Max





Benchmarks
============
(Note that these aren’t very good benchmarks. There’s quite some variation on each run.)

You should exclude your setup out of your actual benchmark, like this:

| numbers |
numbers := (1 to: 1000000) asArray.
[ numbers sum ] benchFor: 10 seconds. 

"a BenchmarkResult(126 iterations in 10 seconds 77 milliseconds. 12.504 per second)"

Machine:
MacBook Pro (15-inch, Early 2011)
CPU: 2.2 GHz Intel Core i7
Memory: 8 GB 1333 MHz DDR3
Disk: APPLE SSD TS512C (500 GB)


Benchmarks of the current versions:

[ (1 to: 1000000) asArray sum ] benchFor: 10 seconds.
75 iterations, 7.470 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ] benchFor: 10 seconds.
72 iterations, 7.128 per second


[ (1 to: 100) asArray sum ] benchFor: 10 seconds.
1,189,477 iterations, 118,912 per second


[ (1 to: 100) asArray sum: [ :e | e ] ] benchFor: 10 seconds.
1,171,467 iterations, 117,112 per second



Benchmarks of the new versions:

[ (1 to: 1000000) asArray sum ] benchFor: 10 seconds.
73 iterations, 7.244 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ] benchFor: 10 seconds.
75 iterations, 7.480 per second

[ (1 to: 1000000) asArray sum: [ :e | e ] ifEmpty: [ 0 ] ] benchFor: 10 seconds.
72 iterations, 7.141 per second


[ (1 to: 100) asArray sum ] benchFor: 10 seconds.
1,115,827 iterations, 111,560 per second

[ (1 to: 100) asArray sum: [ :e | e ] ] benchFor: 10 seconds.
1,154,595 iterations, 115,425 per second

[ (1 to: 100) asArray sum: [ :e | e ] ifEmpty: [ 0 ] ] benchFor: 10 seconds.
1,102,358 iterations, 110,203 per second

On 20 Dec 2015, at 15:26, Ben Coman <[hidden email]> wrote:

On Mon, Dec 21, 2015 at 12:43 AM, Sven Van Caekenberghe <[hidden email]> wrote:

Doru,

For me this whole discussion started because you (the standpoint that you take) hijacked the best selector (#sum) for a use case that is much less common than adding a collection of numbers which should give 0 when empty (you hijack it by not wanting to return 0, which I totally understand, but doing so you redefine the meaning/semantics).

Max's point is to make everything more consistent and remove some API. I support that too.

Now, I like Max's proposal.

But, you know, my conclusion when the thread ended was that it might be better to throw all these selectors away, since #inject:into: covers most use cases at least as well and at least as clear.

Sven

On 20 Dec 2015, at 14:10, Tudor Girba <[hidden email]> wrote:

Hi,

Could we not have sum, but sumNumbers instead? So, we would end up with:

sum:ifEmpty:
sum: (with error)
sumNumbers (without error)

From the outside, #sum: looks like it should parameterize #sum, but the implementation is actually different. So, given that in this implementation #sum is not a special case of #sum: the two should be named differently to reflect that difference. Hence my proposal is to keep #sumNumbers instead of #sum.

I agree somewhat with Duro to avoid #sum and #sum:  having different semantics,
but I agree more with Sven about keeping #sum for numbers only and
returning zero,
so why not turn Doru's suggestion around...

#sum  (empty --> 0)
#sumBy:   (empty --> error)
#sumBy: ifEmpty:   (empty --> no error)

alternatively could be #sumUsing: or #sumWith:  or something similar.

Cheers,
Doru

On Dec 20, 2015, at 1:47 PM, Max Leske <[hidden email]> wrote:

On 20 Dec 2015, at 13:43, Gabriel Cotelli <[hidden email]> wrote:

Max,

sum: aBlock ifEmpty: emptyBlock needs to obtain the sample evaluating the block.

sum: aBlock ifEmpty: emptyBlock
   | sum sample |
   self isEmpty ifTrue: [ ^ emptyBlock value ].
   sample := aBlock value: self anyOne.
   sum := self
           inject: sample
           into: [ :accum :each | accum + (aBlock value: each) ].
   ^ sum - sample

Thanks! Missed that.

On Sun, Dec 20, 2015 at 8:59 AM, Max Leske <[hidden email]> wrote:
I would like to wrap up this discussion.

On 05 Dec 2015, at 18:14, stepharo <[hidden email]> wrote:

So what is the conclusion?
I like the idea of Esteban M to have iterator because it moves some behavior out of core classes.

[[[

aCollection arithmetic sum: [...] or.... aCollection
arithmetic avg.

]]]

While I think that iterators are an intriguing idea I also believe that they are beyond the scope of this issue. If anybody wants to follow up on iterators (or unit types for that matter) please start a new thread / issue.


I propose to use Sven’s version for #sum:ifEmpty:. The result would be these three methods:

sum
   ^ self
           sum: [ :each | each ]
           ifEmpty: [ 0 ]

sum: aBlock
   ^ self
           sum: aBlock
           ifEmpty: [ self errorEmptyCollection ]

sum: aBlock ifEmpty: emptyBlock
   | sum sample |
   self isEmpty ifTrue: [ ^ emptyBlock value ].
   sample := self anyOne.
   sum := self
           inject: sample
           into: [ :accum :each | accum + (aBlock value: each) ].
   ^ sum - sample


I’ve attached a couple of benchmark results below. To me they show that
1. the new implementation is maybe a tiny bit slower but insignificantly so (if you’re going for performance you’ll probably write your own optimised version anyway)
2. there is no need to duplicate the code (like #sum and #sum: currently do). It’s perfectly fine to delegate to #sum:ifEmpty:



In addition to the above changes I would like to remove #detectSum: (-> #sum:) and #sumNumbers (-> #sum).


Note that once we agree on changing this API, we will need to also change #detectMin:, #detectMax:, #min, #max as well as all overrides (e.g. RunArray, Interval) of these and of #sum et. al. to stay consistent. The changes would of course be in line with this change, such that every operation has a unary selector with a sensible default, one that takes a block and throws an error for empty collections and a third that takes a block for the iteration and one for the empty case.


Please cast your vote for these changes:

1. Do you agree to changing #sum and #sum: in the suggested way?

yes

2. Do you agree to the removal of #detectSum:?

?? (not familiar)

3. Do you agree to the removal of #sumNumbers?

yes

4. Do you agree that the #max and #min selectors also need to be adapted?

?? (not familiar)