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. Cheers, Doru > On Dec 20, 2015, at 1:47 PM, Max Leske <[email protected]> wrote: > >> >> On 20 Dec 2015, at 13:43, Gabriel Cotelli <[email protected]> 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 <[email protected]> wrote: >> I would like to wrap up this discussion. >> >> >>> On 05 Dec 2015, at 18:14, stepharo <[email protected]> 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 -- www.tudorgirba.com www.feenk.com "There are no old things, there are only old ways of looking at them."
