Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interval includes: discrepancy #1108

Open
nicolas-cellier-aka-nice opened this issue Mar 1, 2021 · 5 comments
Open

Interval includes: discrepancy #1108

nicolas-cellier-aka-nice opened this issue Mar 1, 2021 · 5 comments
Labels

Comments

@nicolas-cellier-aka-nice
Copy link

nicolas-cellier-aka-nice commented Mar 1, 2021

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
Copy link
Contributor

blairmcg commented Mar 2, 2021

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
Copy link
Contributor

blairmcg commented Mar 2, 2021

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
Copy link
Author

nicolas-cellier-aka-nice commented Mar 2, 2021

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

  • let things as is and advertise that Interval of Float are not recommended and may behave surprisingly
  • try and fix the most inconsistent behaviors

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"

@blairmcg
Copy link
Contributor

blairmcg commented Mar 3, 2021

Yes, that seems like a good approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants