/ http://bugs.squeak.org/view.php?id=7180 +6455 +1603 +1602 />>/ />>/ Patches for Interval indexOf: and includes: />/ />/ I wonder about these fixes because of http://code.google.com/p/pharo/issues/detail?id=35 />/ />/ Did you reconsider the correctness of these fixes? They seem />/ complicated and after reading through the bug reports again />/ (especially http://bugs.squeak.org/view.php?id=7180) I concluded />/ that they may introduce more problems than they fix. /I know I tried to find my way there too. Now I trusted nicolas. Nicolas are you strong on these fixes?
Stef
/ />/ />/ Adrian />/ _______________________________________________ />/ Pharo-project mailing list />/ Pharo-project at lists.gforge.inria.fr <http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project> />/ http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project />
Thank you Stef. Yes/, the fix is safe. I develop here after these 3 points A)- The code was wrong ///B)- Discussions were too long, but the fixes are very easy //C)- library could be enhanced //Adrian, hope I will convince you (also, read the code). /// Cheers Nicolas A) ///The code was wrong// /Prior this patch: ///- #includes: implementation were WRONG. - #indexOf: was missing, falling back to naive super slow implementation /// Try these two tests in any Squeak image of your choice (3.7 to: 3.10): // "There is no reason that the first is true while the second is false" //{(Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 + 1.0e-11. (Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 - 1.0e-11}. / /"There is no reason for this timeToRun to grow" (1 to: 7) collect: [:i | [(1 to: 10000000) indexOf: (10 raisedTo: i)] timeToRun] ///// ////B)- the fixes are very easy//// I simply implemented indexOf: with same algorithm as #includes: (corrected!). Then removed includes: because super includes: is implemented sending indexOf: Then I removed valueIncludes: because includes: was sole sender. You see, two methods removed, one fix added: no revolution / /C)- library could be enhanced /What I would prefer is: - implement a method with an argument so that user can choose comparison tolerance - expand this fuzzy inclusion test to any other collection Maybe something like: includes: aNumber moreOrLess: aTolerance. Then Interval includes: could eventually use a default tolerance. But the obvious bugs must first be corrected... /
Thanks this is great mail! Stef On Oct 9, 2008, at 10:18 PM, nicolas cellier wrote:
http://bugs.squeak.org/view.php?id=7180 +6455 +1603 +1602
Patches for Interval indexOf: and includes:
I wonder about these fixes because of http://code.google.com/p/pharo/issues/detail?id=35
Did you reconsider the correctness of these fixes? They seem complicated and after reading through the bug reports again (especially http://bugs.squeak.org/view.php?id=7180) I concluded that they may introduce more problems than they fix. I know I tried to find my way there too. Now I trusted nicolas. Nicolas are you strong on these fixes?
Stef
Adrian _______________________________________________ Pharo-project mailing list Pharo-project at lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Thank you Stef. Yes, the fix is safe. I develop here after these 3 points A)- The code was wrong B)- Discussions were too long, but the fixes are very easy C)- library could be enhanced
Adrian, hope I will convince you (also, read the code).
Cheers
Nicolas
A) The code was wrong
Prior this patch: - #includes: implementation were WRONG. - #indexOf: was missing, falling back to naive super slow implementation
Try these two tests in any Squeak image of your choice (3.7 to: 3.10):
"There is no reason that the first is true while the second is false" {(Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 + 1.0e-11. (Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 - 1.0e-11}.
"There is no reason for this timeToRun to grow" (1 to: 7) collect: [:i | [(1 to: 10000000) indexOf: (10 raisedTo: i)] timeToRun]
B)- the fixes are very easy
I simply implemented indexOf: with same algorithm as #includes: (corrected!). Then removed includes: because super includes: is implemented sending indexOf: Then I removed valueIncludes: because includes: was sole sender. You see, two methods removed, one fix added: no revolution
C)- library could be enhanced
What I would prefer is: - implement a method with an argument so that user can choose comparison tolerance - expand this fuzzy inclusion test to any other collection Maybe something like: includes: aNumber moreOrLess: aTolerance. Then Interval includes: could eventually use a default tolerance. But the obvious bugs must first be corrected...
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Thanks for the explanation, Nicolas! This sounds reasonable. This kind of conclusion was missing for me when I read through the different bug reports on Mantis. Adrian On Oct 9, 2008, at 22:18 , nicolas cellier wrote:
http://bugs.squeak.org/view.php?id=7180 +6455 +1603 +1602
Patches for Interval indexOf: and includes:
I wonder about these fixes because of http://code.google.com/p/pharo/issues/detail?id=35
Did you reconsider the correctness of these fixes? They seem complicated and after reading through the bug reports again (especially http://bugs.squeak.org/view.php?id=7180) I concluded that they may introduce more problems than they fix. I know I tried to find my way there too. Now I trusted nicolas. Nicolas are you strong on these fixes?
Stef
Adrian _______________________________________________ Pharo-project mailing list Pharo-project at lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Thank you Stef. Yes, the fix is safe. I develop here after these 3 points A)- The code was wrong B)- Discussions were too long, but the fixes are very easy C)- library could be enhanced
Adrian, hope I will convince you (also, read the code).
Cheers
Nicolas
A) The code was wrong
Prior this patch: - #includes: implementation were WRONG. - #indexOf: was missing, falling back to naive super slow implementation
Try these two tests in any Squeak image of your choice (3.7 to: 3.10):
"There is no reason that the first is true while the second is false" {(Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 + 1.0e-11. (Float pi to: 4*Float pi by: Float pi) includes: Float pi*2 - 1.0e-11}.
"There is no reason for this timeToRun to grow" (1 to: 7) collect: [:i | [(1 to: 10000000) indexOf: (10 raisedTo: i)] timeToRun]
B)- the fixes are very easy
I simply implemented indexOf: with same algorithm as #includes: (corrected!). Then removed includes: because super includes: is implemented sending indexOf: Then I removed valueIncludes: because includes: was sole sender. You see, two methods removed, one fix added: no revolution
C)- library could be enhanced
What I would prefer is: - implement a method with an argument so that user can choose comparison tolerance - expand this fuzzy inclusion test to any other collection Maybe something like: includes: aNumber moreOrLess: aTolerance. Then Interval includes: could eventually use a default tolerance. But the obvious bugs must first be corrected...
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
participants (3)
-
Adrian Lienhard -
nicolas cellier -
Stéphane Ducasse