dolphinsmalltalk / Dolphin

Dolphin Smalltalk Core Image
MIT License
301 stars 58 forks source link

Interval includes: discrepancy #1108

Open nicolas-cellier-aka-nice opened 3 years ago

nicolas-cellier-aka-nice commented 3 years ago

Interval size is not consistent with Interval includes:

"This one is true"
(0.3 to: 1.2 by: 0.1) includes: 1.2.

"This one is false"
(0.3 to: 1.2 by: 0.1) anySatisfy: [:each | each = 1.2].

I like very much the definition of includes: from John Brandt, but things are not as consistent as possible.

Similarly, current definition of size can lead to an element greater than the stop bound:

"This is true..."
(0.3 to: 0.82 by: 0.01) anySatisfy: [:each | each > 0.82].

"But this is false because of range check"
(0.3 to: 0.82 by: 0.01) allSatisfy: [:each | (0.3 to: 0.82 by: 0.01) includes: each].

Should size be changed for accomodating the Float case ? For example it could be in case of inexact arithmetic:

candidateSize := (stop - start / step max: 0) rounded.
step > 0
    ifTrue: [candidateSize * step + start <= stop ifTrue: [^candidateSize + 1]]
    ifFalse: [candidateSize * step + start >= stop ifTrue: [^candidateSize + 1]].
^candidateSize
blairmcg commented 3 years ago

Thanks for the bug report Nicolas. From the code history this is obviously of very long standing, but hasn't been reported before. This suggests it is not a common use case, and/or that impact is low. As such, although it would be nice to fix, any fix would have to be very carefully made so as not to significantly impact the performance of more mainstream cases. As Dolphin's VM is an interpreter, the lower layers of the class library need to remain "tight" to keep the system snappy. I'll add some (skipped) tests to the system based on your examples to at least document the deficiency.

blairmcg commented 3 years ago

The 2nd example seems to be a common problem. I get the same incorrect results in Pharo 9.0 (build 1168), and in VW 8,3. Here is another expression of it with results from VW:

interval := (0.3d to: 0.82d by: 0.01d).
interval includes: interval asArray last. "false!"
nicolas-cellier-aka-nice commented 3 years ago

Yes, this is neither specific to Dolphin, nor urgent. The strategies are the following:

In the 2nd option, we do not want to spoil the simplicity and efficiency of the most general case (Interval of Integer or eventually Fraction), so my recommandation would be to create a LimitedPrecisionInterval, and let to: and to:by: choose the adequate species. it's just a matter of testing is step hasLimitedPrecision or: [stop hasLimitedPrecision], and eventually start hasLimitedPrecision if the species dispatch is implemented in Interval class.

Note that stop has to be tested because of size... For example, following Interval is pathological, though containing no Float and having an Integer step...

(1/5 to: 1.2 by: 1) includes: 6/5. "false"
(1/5 to: 1.2 by: 1) asArray last = (6/5). "true"
(1/5 to: 1.2 by: 1) asArray last > 1.2. "true - that's the problem"
nicolas-cellier-aka-nice commented 3 years ago

I gave it a shot in Squeak

https://source.squeak.org/trunk/Kernel-nice.1376.diff https://source.squeak.org/trunk/Collections-nice.925.diff https://source.squeak.org/trunk/CollectionsTests-nice.351.diff

blairmcg commented 3 years ago

Yes, that seems like a good approach.