Pedantic on Random>>nextValue method name

In Random>>nextValue I notice the comment: "This method does NOT
update the seed; repeated sends answer the same value."  Thus the
following produces all the same values...

    oc := OrderedCollection new.
    r := Random new.
    5 timesRepeat: [ oc add: r nextValue ] .
    oc inspect.

Am I being too pedantic thinking this ancient method should be named
#currentValue.  Are there historical constraints to be wary of?  Its
only significant in-Image sender is Random>>next which would then
be...

    ^  (seed := self currentValue) / m

cheers -ben

The method #nextValue actually calculates the next value the seed should be, the current value can be accessed by sending #seed.

I do agree that #nextValue might not be the clearest method name for its use; perhaps #nextSeed would be a better name?

Best regards,
Henrik

On Wed, Sep 16, 2015 at 11:22 PM, Henrik Nergaard <[hidden email]> wrote:
> The method #nextValue actually calculates the next value the seed should be,
> the current value can be accessed by sending #seed.
>
> I do agree that #nextValue might not be the clearest method name for its
> use; perhaps #nextSeed would be a better name?

Yes. Thats better again.  Now its a private method, so there should be
problem changing it ;) ??anyone


Continuing on another Random topic related to..
https://pharo.fogbugz.com/default.asp?16577

I notice that Collection>>atRandom is protected by a mutex but
Collection>>atRandom: is not.  This makes me believe the protection is
for the Random generator and not for the actual picking of the element
from the collection.  So it seems that #atRandom holds the mutex too
long - for both random generation and element picking.

Thus I wonder if rather than copying the existing pattern of
Collection>>atRandom to GlobalSharedRandom>>generateFor:  it might be
better to make a more generic SharedRandom as follows...

"Old calls to #next changed to private #nextValue"

Random>>nextValue
    | lo hi aLoRHi answer |
    hi := (seed quo: q) asFloat.
    lo := seed - (hi * q).  " = seed rem: q"
    aLoRHi := (a * lo) - (r * hi).
    seed := (aLoRHi > 0.0)
         ifTrue:  [aLoRHi]
         ifFalse: [aLoRHi + m].
    ^ seed / m  "this line moved here from old #next"

Random>>next
     ^ self nextValue

Random>>next: anInteger into: anArray
    1 to: anInteger do: [:index | anArray at: index put: self nextValue].
    ^ anArray

Random>>nextInt: anInteger
    anInteger strictlyPositive ifFalse: [ self error: 'Range must be
positive' ].
    anInteger asFloat isInfinite
        ifTrue: [^(self nextValue asFraction * anInteger) truncated + 1].
    ^ (self nextValue * anInteger) truncated + 1


"SharedRandom protects all calls to private #nextValue"

Random subclass: SharedRandom
     instanceVariableNames: 'mutex'

SharedRandom>>initialize
     mutex := Semaphore forMutualExclusion.

SharedRandom>>next
    ^ mutex critical: [ self nextValue ]

SharedRandom>>next: anInteger into: anArray
    ^ mutex critical: [ super next: anInteger into: anArray ].

SharedRandom>>nextInt: anInteger
    ^ mutex critical: [ nextInt: anInteger ].


Then...

Object subclass: GlobalSharedRandom
    classVariables: 'sharedRandom'

GlobalSharedRandom class>>initialize
    sharedRandom := SharedRandom new.

GlobalSharedRandom class>>generator
    ^ sharedRandom


And finally...

Collection>>atRandom
    ^ self randomAt: GlobalSharedRandom generator.


What do you think?
cheers, Ben

-----Original Message-----
From: Pharo-dev [mailto:pharo-dev-bounces@lists.pharo.org] On Behalf Of Ben Coman
Sent: Wednesday, September 16, 2015 6:39 PM
To: Pharo Development List <pharo-dev@lists.pharo.org>
Subject: Re: [Pharo-dev] Pedantic on Random>>nextValue method name

On Wed, Sep 16, 2015 at 11:22 PM, Henrik Nergaard <henrin10@uia.no> wrote:
>> The method #nextValue actually calculates the next value the seed
>> should be, the current value can be accessed by sending #seed.

>> I do agree that #nextValue might not be the clearest method name for
>> its use; perhaps #nextSeed would be a better name?

>Yes. Thats better again.  Now its a private method, so there should be
>problem changing it ;) ??anyone

Continuing on another Random topic related to..
https://pharo.fogbugz.com/default.asp?16577

I notice that Collection>>atRandom is protected by a mutex but
Collection>>atRandom: is not.  This makes me believe the protection is
for the Random generator and not for the actual picking of the element from the collection.  So it seems that #atRandom holds the mutex too long - for both random generation and element picking.
(>
Is there a specific use for the mutex in this case? I tried to use a shared random without the mutex, and forking a few block closures, worked fine..

If the mutex could be removed we could just add a method in the class side of Random like this:

globalGenerator
^Generator ifNil: [ Generator := Random new]

<)

Thus I wonder if rather than copying the existing pattern of
Collection>>atRandom to GlobalSharedRandom>>generateFor:  it might be
better to make a more generic SharedRandom as follows...

(>
Maybe :P

I had a look at the Random Superclass in SciSmalltalk; here #next is used to create and give the next random value. #seed for seed, and #peek for checking what the next value would be without changing the current state.
The peek method could be a good addition in Random ->
        Random>>#Peak
           Self nextValue / m

I would keep the way the #nextValue method behaves since this gives the option to "peek" at the next value the Ivar seed will have without changing it (could be useful?), but the name could be improved to be more clear.
Perhaps the method name #nextSeed might not be so good after all, it might imply that the method creates a new seed, but what it actually does is to go to the next state in the random generation? ( so a better name could be #nextState ?), (perhaps the Ivar seed would also be better called state or internalState? )
       
<)

"Old calls to #next changed to private #nextValue"

Random>>nextValue
    | lo hi aLoRHi answer |
    hi := (seed quo: q) asFloat.
    lo := seed - (hi * q).  " = seed rem: q"
    aLoRHi := (a * lo) - (r * hi).
    seed := (aLoRHi > 0.0)
         ifTrue:  [aLoRHi]
         ifFalse: [aLoRHi + m].
    ^ seed / m  "this line moved here from old #next"

Random>>next
     ^ self nextValue

Random>>next: anInteger into: anArray
    1 to: anInteger do: [:index | anArray at: index put: self nextValue].
    ^ anArray

Random>>nextInt: anInteger
    anInteger strictlyPositive ifFalse: [ self error: 'Range must be positive' ].
    anInteger asFloat isInfinite
        ifTrue: [^(self nextValue asFraction * anInteger) truncated + 1].
    ^ (self nextValue * anInteger) truncated + 1


"SharedRandom protects all calls to private #nextValue"

Random subclass: SharedRandom
     instanceVariableNames: 'mutex'

SharedRandom>>initialize
     mutex := Semaphore forMutualExclusion.

SharedRandom>>next
    ^ mutex critical: [ self nextValue ]

SharedRandom>>next: anInteger into: anArray
    ^ mutex critical: [ super next: anInteger into: anArray ].

SharedRandom>>nextInt: anInteger
    ^ mutex critical: [ nextInt: anInteger ].


Then...

Object subclass: GlobalSharedRandom
    classVariables: 'sharedRandom'

GlobalSharedRandom class>>initialize
    sharedRandom := SharedRandom new.

GlobalSharedRandom class>>generator
    ^ sharedRandom


And finally...

Collection>>atRandom
    ^ self randomAt: GlobalSharedRandom generator. (> +1 <)


What do you think?
cheers, Ben

What do you think?
Best regards,
Henrik

On Thu, Sep 17, 2015 at 7:56 PM, Henrik Nergaard
<[hidden email]> wrote:
I was going to retort that inter-thread race conditions can be tricky
to invoke with considerations of timing and priorities :P  but then I
had the insight this can be done using the debugger as demonstrated
below.

Presuming that use cases exist that require the repeatable nature of a
PRNG. This feature is dependent on the /seed/ ivar having the same
value in the calculation of both /hi/ and /lo/ in #nextValue.  But
consider this sequence of events...

Priority 40 thread1 enters #next, then #nextValue, and takes the
current value of /seed/ to calculate /hi/. Then before proceeding to
the line, priority 50 thread2 enters #next and updates the value of
/seed/.  Thread1 then proceeds to take calculate /lo/ with a different
value of /seed/  -- thus the algorithm is broken.

This might be addressed by caching /seed/ ivar into a temporary
/safeThreadCopySeed/.

Yet other use cases may exist that expect simultaneous threads don't
receive the same random number (more often than single thread would.)
 So priority 40 thread1 enters #next, then #nextValue, and copies the
current value of /seed/ into /safeThreadCopySeed/.  Then before it can
update /seed/, priority 50 thread2 enters #next, then #nextValue, and
copies the same current value of /seed/ into /safeThreadCopySeed/. So
both threads will receive the same random number. To demonstrate...

    | sharedRandom
         context1 process1 thread1 result1
         context2 process2 thread2 result2 |
    sharedRandom := Random new.
    context1 := [ result1 := sharedRandom next ] asContext.
    context2 := [ result2 := sharedRandom next ] asContext.
    process1 := Process forContext: context1 priority: 40.
    process2 := Process forContext: context2 priority: 40.
    thread1 := DebugSession process: process1 context: context1.
    thread2 := DebugSession process: process2 context: context2.
    100 timesRepeat: [
        result1 ifNil: [thread1 stepInto].
        result2 ifNil: [thread2 stepInto].  ].
    Transcript crShow: result1; tab; tab; show: result2.

>
> If the mutex could be removed we could just add a method in the class side
> of Random like this:
>
> globalGenerator
> ^Generator ifNil: [ Generator := Random new]

I don't think Random should have a global. Instead the same on the
proposed SharedRandom might be suitable using a
class-instance-variable...

    SharedRandom class
        instanceVariableNames: 'globalGenerator'

    SharedRandom>>globalGenerator
        ^ globalGenerator ifNil: [ globalGenerator := self new].

Then we can have...

    Collection>>atRandom
        ^ self randomAt: SharedRandom globalGenerator.

cheers -ben