The Inbox: Collections-nice.825.mcz

>
> Nicolas Cellier uploaded a new version of Collections to project The Inbox:
> http://source.squeak.org/inbox/Collections-nice.825.mcz
>
> ==================== Summary ====================
>
> Name: Collections-nice.825
> Author: nice
> Time: 7 April 2019, 11:10:11.564693 pm
> UUID: c9836c60-9acc-45eb-83b2-851f147b70ca
> Ancestors: Collections-nice.824
>
> Try to improve some Interval of Float Behavior.
>
> - change #last so that it agrees with (self at: self size)
>  Note that last behavior slightly changes for empty Interval: (3 to: 0) last is now 2 instead of 0 previously. I don't know of any code impacted, but it's not a completely neutral change
>
> Note: this solves 2 out of the 3 failures of #testIntervalOfFloatLast... The last element might still overshoot the prescribed upper bound. I can also fix it, but implementation of #last is getting overkill.
>
> - implement a ReversedInterval that lazily reverse an Interval of Float, and resolves #testIntervalOfFloatReversed.
>
> The alternative would be resorting to an Array...
>
> - renounce fuzzy inclusion test. As exhibited by #testSurprisingFuzzyInclusionBehavior, it creates more weirdness than it tries to solve. There will be two more test failing, asserting this fuzzy inclusion, but we can later change them.
>
> =============== Diff against Collections-nice.824 ===============
>
> Item was changed:
>   ----- Method: Interval>>indexOf:startingAt: (in category 'accessing') -----
>   indexOf: anElement startingAt: startIndex
>         "startIndex is an positive integer, the collection index where the search is started."
> -       "during the computation of val , floats are only used when the receiver contains floats"
>
> +       | index |
> -       | index val |
>         (self rangeIncludes: anElement) ifFalse: [ ^0 ].
> +       index := (anElement - start / step) rounded + 1.
> -       val := anElement - self first / self increment.
> -       val isFloat
> -               ifTrue: [
> -                       (val - val rounded) abs * 100000000 < 1 ifFalse: [ ^0 ].
> -                       index := val rounded + 1 ]
> -               ifFalse: [
> -                       val isInteger ifFalse: [ ^0 ].
> -                       index := val + 1 ].
> -       "finally, the value of startIndex comes into play:"
>         (index between: startIndex and: self size) ifFalse: [ ^0 ].
> +       (self at: index) = anElement ifFalse: [ ^0 ].
>         ^index!
>
> Item was changed:
>   ----- Method: Interval>>last (in category 'accessing') -----
>   last
>         "Refer to the comment in SequenceableCollection|last."
>
> +       ^start + (step * (self size - 1))!
> -       ^stop - (stop - start \\ step)!
>
> Item was changed:
>   ----- Method: Interval>>reversed (in category 'converting') -----
>   reversed
>         self isEmpty ifTrue: [^stop to: start by: step negated].
> +       (start isFraction and: [step isFraction]) ifFalse: [^ReversedInterval newFrom: self].
>         ^self last to: start by: step negated!
>
> Item was added:
> + SequenceableCollection subclass: #ReversedInterval
> +       instanceVariableNames: 'original'
> +       classVariableNames: ''
> +       poolDictionaries: ''
> +       category: 'Collections-Sequenceable'!
> +
> + !ReversedInterval commentStamp: 'nice 4/6/2019 16:30' prior: 0!
> + A ReversedInterval is an Interval reversed.
> + An Interval reversed is generally an Interval, except when made of Float.
> + This is because Float arithmetic does not guarantee reversible operations.
> +
> + Instead of transforming the reversed Float Interval into an Array, I keep a lazier and cheaper representation.
> + I can even pretend being an Interval myself. As long as I quake like a duck...
> +
> + Instance Variables
> +       original:               <Interval>
> +
> + original
> +       - the Interval being reversed
> + !
>
> Item was added:
> + ----- Method: ReversedInterval class>>newFrom: (in category 'as yet unclassified') -----
> + newFrom: aCollection
> +       "Answer an instance of me containing the same elements as aCollection, reversed."
> +
> +       aCollection isInterval ifTrue: [^self basicNew setOriginal: aCollection].
> +       ^(Interval newFrom: aCollection) reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>+ (in category 'arithmetic') -----
> + + aNumber
> +       ^(original + aNumber) reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>- (in category 'arithmetic') -----
> + - aNumber
> +       ^(original - aNumber) reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>= (in category 'comparing') -----
> + = anObject
> +       ^ self == anObject
> +               or: [anObject class = self class
> +                       and: [original = anObject reversed]]!
>
> Item was added:
> + ----- Method: ReversedInterval>>at: (in category 'accessing') -----
> + at: anInteger
> +       ^original at: original size + 1 - anInteger!
>
> Item was added:
> + ----- Method: ReversedInterval>>copyFrom:to: (in category 'copying') -----
> + copyFrom: startIndex to: stopIndex
> +       (startIndex = 1 and: [stopIndex = self size]) ifTrue: [^self].
> +       stopIndex < startIndex ifTrue: [^self copyEmpty].
> +       ^((self at: stopIndex) to: (self at: startIndex) by: original increment) reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>do: (in category 'enumerating') -----
> + do: aBlock
> +       original reverseDo: aBlock!
>
> Item was added:
> + ----- Method: ReversedInterval>>hash (in category 'comparing') -----
> + hash
> +       ^original hash bitXor: SmallInteger maxVal!
>
> Item was added:
> + ----- Method: ReversedInterval>>isEmpty (in category 'testing') -----
> + isEmpty
> +       ^self size = 0!
>
> Item was added:
> + ----- Method: ReversedInterval>>printOn: (in category 'printing') -----
> + printOn: aStream
> +       aStream
> +               nextPut: $(;
> +               print: original;
> +               nextPut: $);
> +               space;
> +               nextPutAll: #reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>reverseDo: (in category 'enumerating') -----
> + reverseDo: aBlock
> +       original do: aBlock!
>
> Item was added:
> + ----- Method: ReversedInterval>>reversed (in category 'converting') -----
> + reversed
> +       ^original!
>
> Item was added:
> + ----- Method: ReversedInterval>>setOriginal: (in category 'private') -----
> + setOriginal: anInterval
> +       original := anInterval!
>
> Item was added:
> + ----- Method: ReversedInterval>>size (in category 'accessing') -----
> + size
> +       ^original size!
>
> Item was added:
> + ----- Method: ReversedInterval>>sorted (in category 'sorting') -----
> + sorted
> +       original increment > 0 ifTrue: [^original].
> +       ^self!
>
> Item was added:
> + ----- Method: ReversedInterval>>species (in category 'private') -----
> + species
> +
> +       ^original species!
>
> Item was added:
> + ----- Method: ReversedInterval>>storeOn: (in category 'printing') -----
> + storeOn: aStream
> +       aStream
> +               nextPut: $(;
> +               store: original;
> +               nextPut: $);
> +               space;
> +               nextPutAll: #reversed!
>
> Item was added:
> + ----- Method: ReversedInterval>>sum (in category 'accessing') -----
> + sum
> +       ^original sum!
>
>

> On 13.09.2019, at 08:50, Fabio Niephaus <[hidden email]> wrote:
>
>
>
> On Thu, 12 Sep 2019 at 1:02 am, Chris Muller <[hidden email]> wrote:
> "An Interval reversed is generally an Interval, except when made of Float."
> Should ReversedInterval be renamed to ReversedFloatInterval then?
>
> +1
>
> Interval was designed for Integers only.  IMO, it should be copied to a separate class, FloatInterval, to implement this capability.
>
> Does anyone actually need Float support in Interval at this point? Or would it make sense to just disallow Floats in Interval for the time being?
>

> Fabio
>
>
>  
>
> Nonetheless, this seems to fix #testIntervalOfFloatLast and
> #testIntervalOfFloatReversed of IntervalTest. Do we want to integrate
> this proposal or shall we mark these two tests as expected failures?
>
> Fabio
>
>
>

>
>
> > On 13.09.2019, at 08:50, Fabio Niephaus <[hidden email]> wrote:
> >
> >
> >
> > On Thu, 12 Sep 2019 at 1:02 am, Chris Muller <[hidden email]> wrote:
> > "An Interval reversed is generally an Interval, except when made of Float."
> > Should ReversedInterval be renamed to ReversedFloatInterval then?
> >
> > +1
> >
> > Interval was designed for Integers only.  IMO, it should be copied to a separate class, FloatInterval, to implement this capability.
> >
> > Does anyone actually need Float support in Interval at this point? Or would it make sense to just disallow Floats in Interval for the time being?
> >
>
> I don't see why supporting
>
>         2.4 to: 4.5
>
> would be a bad idea. At least, it has a proper mathematical meaning.
> Question is, whether just
>
>         (1.0 to: 3.0) includes: 2.5
>
> should be true or also
>
>         (1 to: 3) includes: 2.5
>
> :D

>
> > Fabio
> >
> >
> >
> >
> > Nonetheless, this seems to fix #testIntervalOfFloatLast and
> > #testIntervalOfFloatReversed of IntervalTest. Do we want to integrate
> > this proposal or shall we mark these two tests as expected failures?
> >
> > Fabio
> >
> >
> >
>
>
>

> On 13.09.2019, at 11:47, Nicolas Cellier <[hidden email]> wrote:
>
> Interval of Float are not nice to have: we already have!
> Here are 4 propositions that I wanted to put on the table:
> 1) It is recommended to avoid using Interval of Float, because they will have pitfalls, expecially if you have naive expectations.
> 2) It is void to try masking the inexact nature of Float by partial waorkarounds
> 3) It is recommended to provide implementation robust to inexact arithmetic, and avoid adding more pitfalls than necessary.
> 4) It is recommended to teach the limitations.
>     Known failures is somehow a way to document that.
>
> But adding some code to explicitly forbid Interval of Float, no, that's something I would avoid!
> IMO, it is a wrong answer to 1).
> It's like wanting to over-protect the user because we presume he is dumb and cannot learn anything complex.
> That's not the spirit of Smalltalk IMO. We must make simple things simple, and open the possibility to make more complex things.
> We must not be castrating but inviting and teaching.
> After all, making mistakes is a good way to learn, we must not try to prevent that ;)
> IMO the right answer is 4).
>
> Float are full of pitfalls. But we don't forbid Float, do we? They have too much practical value, and we'd better learn to use them.
> Maybe Interval of Float has less value than Float, but it's a bit like saying that oranges have less value than citruses ;)
> They have not much value in the core image, but you cannot presume they will never have.
> Try to make a LogarithmicInterval without Float for example... Analysis of systems in frequency domain requires such species!
>
> The dicussion I wanted to open was about 2) and 3).
> In the past, some wanted to reduce the surprising behavior by masking the inexact nature of Float arithmetic
> (round to nearest in the best case, uncontrolled accumulation of rounding errors in the worse one).
> This was done by encoding arbitrary comparison thresholds in some methods.
> But this lead to more surprising behavior in the end, and it is enforcing wrong expectations.
> That's what I tried to document with the new failing tests.
>
> So the first thing is that I wanted to remove those workarounds, and reveal the true nature of Float, not masking it.
> They make things worse IMO.
>
> The second thing is that there are other naive implementations of Interval methods that won't work with inexact arithmetic.
> But with some care, we can rewrite those to also behave better with inexact. The question if it is worth or not.
> It's a trade off between complexity and utility.
> That's why it is in the inbox, and we can decide that they are nice to have or overkill for too few value.
> We can also postpone the decision with known failures, fix later, or delegate to an external package...

>
>
> Le ven. 13 sept. 2019 à 10:26, Fabio Niephaus <[hidden email]> a écrit :
> On Fri, Sep 13, 2019 at 9:57 AM Tobias Pape <[hidden email]> wrote:
> >
> >
> > > On 13.09.2019, at 08:50, Fabio Niephaus <[hidden email]> wrote:
> > >
> > >
> > >
> > > On Thu, 12 Sep 2019 at 1:02 am, Chris Muller <[hidden email]> wrote:
> > > "An Interval reversed is generally an Interval, except when made of Float."
> > > Should ReversedInterval be renamed to ReversedFloatInterval then?
> > >
> > > +1
> > >
> > > Interval was designed for Integers only.  IMO, it should be copied to a separate class, FloatInterval, to implement this capability.
> > >
> > > Does anyone actually need Float support in Interval at this point? Or would it make sense to just disallow Floats in Interval for the time being?
> > >
> >
> > I don't see why supporting
> >
> >         2.4 to: 4.5
> >
> > would be a bad idea. At least, it has a proper mathematical meaning.
> > Question is, whether just
> >
> >         (1.0 to: 3.0) includes: 2.5
> >
> > should be true or also
> >
> >         (1 to: 3) includes: 2.5
> >
> > :D
>
> I didn't say it's a bad idea, I wanted to know if someone actually
> needs it right now. If not, I'd just flag this as a "nice to have"
> feature we could support in a future release, but not the one we are
> working on at the moment.
>
> Fabio
>
> >
> > > Fabio
> > >
> > >
> > >
> > >
> > > Nonetheless, this seems to fix #testIntervalOfFloatLast and
> > > #testIntervalOfFloatReversed of IntervalTest. Do we want to integrate
> > > this proposal or shall we mark these two tests as expected failures?
> > >
> > > Fabio
> > >
> > >
> > >
> >
> >
> >
>
>