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. |
|