0006560: [BUG][FIX] "{Color red. Color green. Color blue} sum" can't return "Color white"

(0010875 - 405 - 507 - 507 - 507 - 507 - 507)
wiz
07-14-07 04:37
edited on: 07-14-07 04:40

Hi sumi,

A great bug find.

Looking at your fix, i realized
injecting (sample - sample) gives you the zero element . So I tried that and wrote some tests to prove it still worked.

Uploaded are the fix
Collection-sum-wiz.st

and bug tests
SumBugs.st

the four color sum test fail until the patch is provided.

If you can think of other ways to torture sum, please add to the tests.

(0010878 - 75 - 81 - 81 - 81 - 81 - 81)
sumi
07-14-07 11:23

Thank you for your response.
The injecting (sample - sample) is nice idea!

(0010885 - 141 - 207 - 207 - 207 - 207 - 207)
wiz
07-17-07 03:43
edited on: 07-17-07 03:44

"fix begin"
Installer mantis bug: 6560 fix: 'Collection-sum-wiz.st'.
"fix test"
Installer mantis bug: 6560 fix: 'SumBugs.st'.
"fix end"

(0012484 - 716 - 1143 - 1143 - 1143 - 1143 - 1143)
jbj
08-12-08 07:23

Sorry, but I don't like the solution here. The case obviously calls for a implementation that doesn't use #inject:into:.

A cleaner solution is to add a #reduce: method to Collection or SequencableCollection and implement #sum in terms of that.

Just to be clear, there is a good implementation of #reduce in the Magritte-model package that looks like so:

reduce: aBlock
    | result |
    result := self first.
    2 to: self size do: [ :index |
        result := aBlock
            value: result
            value: (self at: index) ].
    ^ result

So once we have this method in core, #sum can be simply:

sum
     ^ self reduce: [:accum :each| accum + each ]

Simple, well factored and elegant with no unnecessary work done.

(0012485 - 386 - 454 - 454 - 454 - 454 - 454)
sumi
08-12-08 10:33

I think introducing #reduce: (or a sort of #inject:into: without initial value argument) is very nice idea. But your implementation is not suitable.

Both of your #sum and #reduce: don't work properly as you wish. Please try below.

| set |
set := {Color red. Color green. Color green. Color blue} asSet.
set sum "=> Error "
set reduce: [:sum :each | sum + each] "=> Error "

(0012502 - 1322 - 1600 - 1600 - 1600 - 1600 - 1600)
wiz
08-14-08 03:23
edited on: 08-14-08 03:28

Hi guys,

I note that the discussion on squeak dev took an early academic turn.

When I got to this problem I was just trying to fix a bug and write a test to prove that
a) there was a bug (run test w/o patch)
b) the patch fixed it.

The rest was just.
1) Do the simpliest thing that might work.
2) Respect my time and other projects by leaving it at that,
Respecting the "your-not-gonna-need-it " principle.

3) There is also an implicity rule to respect the external language of squeak.
That is, if a message is used for a specific behavior. Then the fix needs to preserve that meaning and behavior.

Here it means sum works for all collections so the fix needs to as well.
It has to retain its generality.

4) Elegance and economy are also important.
Intention revealing code. Simplicity.

4a) going easy on the gc.

The reduce code looks sort of cool.
Sumi's points out that it raises errors when used for sum.
This suggest that sum needs an sunit test
that tests many cases of collection.

This would apply to reduce as well if it also were spec'ed to be general.

So, jbj, believe in your idea enough to make some working code and tests that prove it works?

I would suggest you offer it in its own report as an [enh] .

Yours is service and curiosity, --Jerome Peace

(0013458 - 139 - 193 - 193 - 193 - 193 - 193)
wiz
01-09-10 02:49

"fix begin"
Installer mantis bug: 6560 fix: 'Collection-sum-wiz.st'.
"fix test"
Installer mantis bug: 6560 fix: 'SumBugs.st'.
"fix end"

(0013459 - 292 - 339 - 339 - 339 - 339 - 339)
wiz
01-09-10 02:53

This bug still exist in Squeak3.11-8720-alpha.image!
Which can be seen by loading and running the tests.

Well it has been over a year since I last responded.

My recommendation is to harvest the fix I proposed. It is sound and valid.

Yours in curiosity and service, --Jerome Peace

(0013749 - 33 - 33 - 33 - 33 - 33 - 33)
sumi
05-04-10 15:25

This bug still exist in Squeak4.1

(0013750 - 50 - 50 - 50 - 50 - 50 - 50)
andreas
05-04-10 15:57

Thanks for the reminder. It is now fixed in trunk.

(0013753 - 353 - 400 - 400 - 400 - 400 - 400)
sumi
05-06-10 07:22

I also fix the Bag>>#sum in the same wiz's manner (sample-sample for generate zero element) and attached as Bag-sum-sumi-M6560.st.

"example"
{Color red muchDarker. Color green muchDarker. Color red muchDarker. Color blue muchDarker. Color blue muchDarker. Color green muchDarker} asBag sum "=> Color white is more desirable than raising exception"

(0013758 - 73 - 79 - 79 - 79 - 79 - 79)
sumi
05-06-10 11:56

I notice that Bag>>#sum has the same problem. Please fix it also. Thanks.

(0013905 - 43 - 49 - 49 - 49 - 49 - 49)
leves
11-07-10 01:00

Bag >> #sum is fixed in Collections-ul.403.

Issue History
Date Modified	Username	Field	Change
07-13-07 08:47	sumi	New Issue
07-13-07 08:47	sumi	File Added: Collection-sum.st.gz
07-13-07 09:11	sumi	Issue Monitored: sumi
07-14-07 04:37	wiz	Note Added: 0010875
07-14-07 04:37	wiz	File Added: Collection-sum-wiz.st
07-14-07 04:37	wiz	File Added: SumBugs.st
07-14-07 04:40	wiz	Note Edited: 0010875
07-14-07 11:23	sumi	Note Added: 0010878
07-17-07 03:43	wiz	Note Added: 0010885
07-17-07 03:43	wiz	Note Edited: 0010885
07-17-07 03:44	wiz	Note Edited: 0010885
08-12-08 07:23	jbj	Note Added: 0012484
08-12-08 10:33	sumi	Note Added: 0012485
08-14-08 03:23	wiz	Note Added: 0012502
08-14-08 03:28	wiz	Note Edited: 0012502
01-10-09 02:00	Keith_Hodges	Status	new => pending
01-10-09 02:27	Keith_Hodges	Status	pending => testing
01-10-09 03:39	Keith_Hodges	Status	testing => resolved
01-10-09 03:39	Keith_Hodges	Fixed in Version	=> 3.11
01-10-09 03:39	Keith_Hodges	Resolution	open => fixed
01-10-09 03:39	Keith_Hodges	Assigned To	=> Keith_Hodges
01-10-09 03:41	Keith_Hodges	Status	resolved => testing
10-03-09 19:34	Keith_Hodges	Status	testing => assigned
10-03-09 19:34	Keith_Hodges	Assigned To	Keith_Hodges => andreas
01-09-10 02:49	wiz	Note Added: 0013458
01-09-10 02:53	wiz	Note Added: 0013459
05-04-10 15:25	sumi	Note Added: 0013749
05-04-10 15:57	andreas	Status	assigned => resolved
05-04-10 15:57	andreas	Fixed in Version	3.11 => trunk
05-04-10 15:57	andreas	Note Added: 0013750
05-06-10 07:10	sumi	File Added: Bag-sum-sumi-M6560.st
05-06-10 07:22	sumi	Note Added: 0013753
05-06-10 08:35	FrankShearar	Relationship added	has duplicate 0007526
05-06-10 11:56	sumi	Status	resolved => feedback
05-06-10 11:56	sumi	Resolution	fixed => reopened
05-06-10 11:56	sumi	Note Added: 0013758
11-07-10 01:00	leves	Status	feedback => resolved
11-07-10 01:00	leves	Resolution	reopened => fixed
11-07-10 01:00	leves	Note Added: 0013905
02-06-11 23:48	leves	Status	resolved => closed

Notes
(0010875 - 405 - 507 - 507 - 507 - 507 - 507) wiz 07-14-07 04:37 edited on: 07-14-07 04:40	Hi sumi, A great bug find. Looking at your fix, i realized injecting (sample - sample) gives you the zero element . So I tried that and wrote some tests to prove it still worked. Uploaded are the fix Collection-sum-wiz.st and bug tests SumBugs.st the four color sum test fail until the patch is provided. If you can think of other ways to torture sum, please add to the tests.

(0010878 - 75 - 81 - 81 - 81 - 81 - 81) sumi 07-14-07 11:23	Thank you for your response. The injecting (sample - sample) is nice idea!

(0010885 - 141 - 207 - 207 - 207 - 207 - 207) wiz 07-17-07 03:43 edited on: 07-17-07 03:44	"fix begin" Installer mantis bug: 6560 fix: 'Collection-sum-wiz.st'. "fix test" Installer mantis bug: 6560 fix: 'SumBugs.st'. "fix end"

(0012484 - 716 - 1143 - 1143 - 1143 - 1143 - 1143) jbj 08-12-08 07:23	Sorry, but I don't like the solution here. The case obviously calls for a implementation that doesn't use #inject:into:. A cleaner solution is to add a #reduce: method to Collection or SequencableCollection and implement #sum in terms of that. Just to be clear, there is a good implementation of #reduce in the Magritte-model package that looks like so: reduce: aBlock \| result \| result := self first. 2 to: self size do: [ :index \| result := aBlock value: result value: (self at: index) ]. ^ result So once we have this method in core, #sum can be simply: sum ^ self reduce: [:accum :each\| accum + each ] Simple, well factored and elegant with no unnecessary work done.

(0012485 - 386 - 454 - 454 - 454 - 454 - 454) sumi 08-12-08 10:33	I think introducing #reduce: (or a sort of #inject:into: without initial value argument) is very nice idea. But your implementation is not suitable. Both of your #sum and #reduce: don't work properly as you wish. Please try below. \| set \| set := {Color red. Color green. Color green. Color blue} asSet. set sum "=> Error " set reduce: [:sum :each \| sum + each] "=> Error "

(0012502 - 1322 - 1600 - 1600 - 1600 - 1600 - 1600) wiz 08-14-08 03:23 edited on: 08-14-08 03:28	Hi guys, I note that the discussion on squeak dev took an early academic turn. When I got to this problem I was just trying to fix a bug and write a test to prove that a) there was a bug (run test w/o patch) b) the patch fixed it. The rest was just. 1) Do the simpliest thing that might work. 2) Respect my time and other projects by leaving it at that, Respecting the "your-not-gonna-need-it " principle. 3) There is also an implicity rule to respect the external language of squeak. That is, if a message is used for a specific behavior. Then the fix needs to preserve that meaning and behavior. Here it means sum works for all collections so the fix needs to as well. It has to retain its generality. 4) Elegance and economy are also important. Intention revealing code. Simplicity. 4a) going easy on the gc. The reduce code looks sort of cool. Sumi's points out that it raises errors when used for sum. This suggest that sum needs an sunit test that tests many cases of collection. This would apply to reduce as well if it also were spec'ed to be general. So, jbj, believe in your idea enough to make some working code and tests that prove it works? I would suggest you offer it in its own report as an [enh] . Yours is service and curiosity, --Jerome Peace

(0013458 - 139 - 193 - 193 - 193 - 193 - 193) wiz 01-09-10 02:49	"fix begin" Installer mantis bug: 6560 fix: 'Collection-sum-wiz.st'. "fix test" Installer mantis bug: 6560 fix: 'SumBugs.st'. "fix end"

(0013459 - 292 - 339 - 339 - 339 - 339 - 339) wiz 01-09-10 02:53	This bug still exist in Squeak3.11-8720-alpha.image! Which can be seen by loading and running the tests. Well it has been over a year since I last responded. My recommendation is to harvest the fix I proposed. It is sound and valid. Yours in curiosity and service, --Jerome Peace

(0013749 - 33 - 33 - 33 - 33 - 33 - 33) sumi 05-04-10 15:25	This bug still exist in Squeak4.1

(0013750 - 50 - 50 - 50 - 50 - 50 - 50) andreas 05-04-10 15:57	Thanks for the reminder. It is now fixed in trunk.

(0013753 - 353 - 400 - 400 - 400 - 400 - 400) sumi 05-06-10 07:22	I also fix the Bag>>#sum in the same wiz's manner (sample-sample for generate zero element) and attached as Bag-sum-sumi-M6560.st. "example" {Color red muchDarker. Color green muchDarker. Color red muchDarker. Color blue muchDarker. Color blue muchDarker. Color green muchDarker} asBag sum "=> Color white is more desirable than raising exception"

(0013758 - 73 - 79 - 79 - 79 - 79 - 79) sumi 05-06-10 11:56	I notice that Bag>>#sum has the same problem. Please fix it also. Thanks.

(0013905 - 43 - 49 - 49 - 49 - 49 - 49) leves 11-07-10 01:00	Bag >> #sum is fixed in Collections-ul.403.