[Ns-developers] Random Variables API changes

[Mathieu Lacage]

On Fri, 2008-10-31 at 16:51 -0400, Raj Bhattacharjea wrote:
> On Wed, Oct 22, 2008 at 3:45 PM, Raj Bhattacharjea <raj.b at gatech.edu>
> wrote:
> > Including ns-developers in the reply list for this off-list mail...
> >
> > On Thu, Oct 16, 2008 at 12:52 PM, Hadi Arbabi <hadiarbabi at gmail.com>
> wrote:
> >> Dear Raj,
> >>
> >>  I have attached codes for ns3 random variable with changes done.
> >>
> >
> > I will post a repo with Hadi's changes for review.
> 
> I have posted this repo (sorry for the delay Craig).
> 
> http://code.nsnam.org/raj/ns-3-rng-changes/
> 
> Please see the tip changeset:
> http://code.nsnam.org/raj/ns-3-rng-changes/rev/c9fee5b56a96
> 
> Michelle's student Hadi prepared this, I simply posted it for him
> (with some minor changes to end of line encoding and removing some
> unneeded "ns3::" scope).
> 
> As far as I can tell, it currently doesn't compile, but I figure that
> getting the jist of the patch out to developers for review was
> important before merge week began.  I will summarize the list of
> changes that I can see in the patchset:
> 
> 1.  RandomVariableBase::UseDevRandom and associated functionality has
> been removed.
> 2.  RandomVariableBase has new APIs for getting the seed and run
> number.
> 3.  Functionality of time-based seeding has been removed.
> 4.  All RandomVariable classes have removed the GetSIngleValue API
> 5.  UniformVariable has a new API which is like GetSingleValue
> 6.  The Box-Muller transform algorithm for NormalVariable generation
> is modified (?)
> 
> It would be useful if Hadi and/or Michelle could comment on these on
> the list.  My one concern with the changes lies in the fact that the
> UniformVariable retains a "GetSingleValue" type of API, while no other
> class does.

This specific API addition looks good to me. The rest of the API, is
however getting increasingly out of control: it lacks coherency and
makes for a really-hard-to-read header.

API comments below:

1) These two methods:

  static void GetGlobalSeedArray(uint32_t seeds[]);
  static bool CheckSeedArray(uint32_t seed[]);

should be instead:

  static void GetGlobalSeedArray(uint32_t seeds[6]);
  static bool CheckSeedArray(uint32_t seed[6]);

(yes, this really does what you want because C and C++ implicitely
convert arrays to pointers during function calls so the above is
semantically equivalent to the current version but it is safer because
it allows the compiler to do more error checking and is
self-documenting).

2) This method:
static void UseGlobalSeed(uint32_t s0, uint32_t s1, uint32_t s2, 
                            uint32_t s3, uint32_t s4, uint32_t s5);

should match the signature of the new methods GetGlobalSeeArray:

static void UseGlobalSeed(uint32_t seed[6]);

3) This method should be removed:

static uint32_t GetGlobalSeed(int index);

because it duplicates 

static void GetGlobalSeedArray(uint32_t seeds[]);

4) I think that the global keyword is not very helpful in conveying the
semantics of these methods, especially since the SetRunNumber and
GetRunNumber methods don't use it. I would like to suggest moving the
methods which act on global scope to a separate class.

For example, maybe this would make sense (better class name welcome):

// manage all global seed-related state.
class SeedManager
{
public:
  // users can override the global default.
  static void SetSeed (uint32_t seed);
  static uint32_t GetSeed (void);
  static void SetSeed (uint32_t seed[6]);
  static void GetSeed (uint32_t seed[6]);
  static void SetRun (uint32_t run);
  static uint32_t GetRun (void);
  static bool CheckSeed (uint32_t seed);
  static bool CheckSeed (uint32_t seed[6]);

  // used to allocate a new seed, return it, and update the 
  // global seed state.
  static void AllocateSeed (uint32_t seed[6]);
};

5) Lacks ability to override per-variable seed:

class RandomVariable
{
public:
void SetSeed (uint32_t seed);
};
(don't care about a more generic version uint32_t seed[6])


Implementation comments:

1) I see that some of the devrandom code was not removed even though the
relevant API was removed. It would really help to remove that code to
improve the code readability (see RandomVariableBase::devRandom).

2) It would really help to move the global state implementation to a
separate class so, the suggestion in (4) above to move all global API to
a separate class would also improve the readability of the
implementation in my opinion.

Finally, I did not spot any support for command-line/easy control of
seeds so, I would like to suggest the following:

1) allow getting the global seed+run number from an env var. Suggested
format: NS_RNG=seed:run where seed is a single 32 bit number. Also
support NS_RNG=print.

2) support command-line options which override NS_RNG env var.
  --RngSeed=seed (seed is a single 32 bit number)
  --RngRun=run
No need to support command-line control of the full 6-32bit seed since
we have API for this for those who want it. To implement this, use
GlobalValue. Example located in src/simulator/simulator.cc
(g_simTypeImpl). The only slightly tricky issue is that you need to read
the initial value from NS_RNG, and, if it is not set, use a default.

3) To make it possible to change the seed of a specific RandomVariable
instance using GtkConfigStore and with Config::Set ("/...", ..), you
need to change the implementation of "std::istream &operator >>
(std::istream &is, RandomVariable &var)" to handle the syntax:
"Seed=value:Type:args"

Basically, you must scan the input string for ":". If the string before
":" contains an "=", use the right hand side of the "=" expression as a
single integer seed, and call RandomVariable.SetSeed once the variable
is created.

I hope that the above helps,

regards,
Mathieu