Re: [boost] Random Review

From: Jens Maurer (jmaurer_at_[hidden])
Date: 2000-06-08 17:08:02

Thank you for your extensive and detailed review.

Thomas Holenstein wrote:
>  (2) Remove the iterator interface of your function objects

I had my library without the iterator interface first and added it
because boost members, including Beman Dawes, wanted it.
It seemed "natural" and useful to them.  Those who wanted
it were certainly aware that there is some overlap in interface
functionality between a 0-ary function (i.e. a Generator) and an
input iterator.
 
>  (3) The distributions should be default constructable where possible.
>      I would like to be able to write:
>        boost::uniform_01<boost::mt19937> rand;

This doesn't work with the current architecture, because all
distributions have references to the underlying generators,
and not copies.  See random-concepts.html for the reasoning.
With a default constructor, the reference must be initialized to
something, and I don't see what this something should be.

> If you do not want to remove the iterator interface, then I think
> "operator();" should behave like "operator*(); operator++();".
> Unfortunately, the following two programs do *not* generate the same
> output, as I would expect:

Hm... This is most easily fixed if we insist on input iterator
semantics, i.e. operator* and operator++ must be called
alternatingly.  Then we can make operator++ a NOP and operator*
just calls operator().  This also saves the extra member variable
to hold the value for operator* (see generator_iterator_mixin_adapter).

> I would also prefer the exclusive maximum for floating point generators.

Noted.

> Streamable concept:
> -------------------
> Why are distributions not streamable?

Good question.  normal_distribution has internal state of a
similar quality as mersenne_twister, for example.
Saving and restoring a distribution's parameters and state without
the underlying generator is easy and consistent.  However, also
saving a distribution's underlying generator poses a problem when
restoring: A distribution has a reference to its generator, which may
be used by several distributions.  What's the semantics of a
restore then? The last distribution restored, wins?  Not my
preference.

We should think carefully about this and consider adding these
features later (after the initial release of the library).
As Beman said, it's getting a bit over-engineered anyway.

> Nondet random clearly misses member function min and max.

Consider it fixed.

> The patch "config.hpp.diff" fails.  It seems to me that it has not to
> + be applied in the current version.

The patches will not be included in the release version, because they
should have been applied one way or another.
  
> I did not find BOOST_STDINT_H_HAS_UINT64_T defined anywhere, so I could
> not test rand48 at first.  I defined it myself, it did not compile, because of
> some missing operator>>.

Your standard library probably has no I/O operators for int64_t
(i.e. long long) and uint64_t.

> random_test.cpp compiled well too.  Note: there were many buckets with
> too great distance.  Especially the 2nd last one had about 30
> (guessed) buckets which were out of the confidence interval (don't
> know how normal this is...)

It is very normal.  The distribution used to fill the buckets is
constructed of several stages of uniform_int<> distributions,
so the quality seems to get worse.

> I think the documentation still needs some work.  Probably some simple
> examples would be useful, for people which just want some quick and
> easy random numbers.

I'll see what I can do for a tutorial.  Not today, though.

> random-concepts.html
> --------------------
> 
> I'm not sure how a user has to provide a numeric_limits<T>, but afaik
> he is not allowed to put anything in namespace std.  So probably (?) the
> first table means numeric_limits<T>::is_specialized is true.

You are allowed to specialize classes in std:: if the specialization
involves a user-defined type.

> random-generators.html
> ----------------------
> This documentation has a small flaw because my netscape nearly does not
> distinguish between <h2> and <h3> (this is, of course not your fault).
> May be you could insert a <hr> before a <h2>.

No, fix your netscape. And mine, too, please. :-)

> I would not document the modulus arithmetic class, as I think this is
> *not* in the interface you provide.

It is clearly placed in the "detail" namespace, as are all other
"generic" templates, such as linear_congruential, so it's not
publicly exported.  I don't see why documentation hurts?

Basically, there are two classes of users for my library:
 - using-only: "I want to simulate rolling a die."
 - creating: "How does a linear congruential generator behave for
these newly thought of input parameters?"

The first type of user should be protected from too much freedom
to shoot in his own foot, so "namespace detail" is off-limits.

The second type of user is not explicitly addressed, but those
people can and should use the documented, but implementation-only
classes.  They should know how to avoid the pitfalls.

> In the table, generators seem to be faster the larger the "approximate
> relative speed" is.  This makes sense for me, but I think "approximate
> relative running time" would be more intuitive, as I am used to measure
> performance in seconds.  So probably you should use the table on the bottom.

I don't care one way or the other.  What do the others think?

> I would prefere the hyperlinks from the top not going to the specializations
> but to the beginning of the paragraph.  I.e. the link in "minstd_rand" should
> go to detail::linear_congruential.

Other opinions?

> The note in detail::linear_congruential probably means minstd_rand, and not
> rand48, does it?

Consider it fixed.

> In section detail::inversive_congruential you write "Instantiations of
> class template mersenne_twister model..." instead of inversive_congruential.

Cut & paste screw-up.
 
> random-distributions.html
> -------------------------
> 
> Seems very good.  Maybe, the corresponding constructor call could go into
> the table on the top for each distribution.

The table on top is for answering the question: "What distribution
do I probably need for my current problem?", so constructor parameters
probably only obscure the issue.

> lognormal_distribution: [...] 
> uniform_sphere: Detail: lib.alg.generate seems to be 25.2.6, not 26.2.6.

Consider these items fixed.

> Implementation:
> ===============
> 
> These are only minor and short:
>   - Do we need to include <string>?

I don't think so. Consider it removed.

>   - You should document (with few words) that has_fixed_range is also
>     compiler dependent.

Yes.

>   - Line 745:  enum { has_fixed_range = true }; probably should be
>                enum { has_fixed_range = false};

Yes.

>   - What purpose serves generator_reference_t anyhow?
>     I did not find it documented nor used in random.hpp.

In my experiments, I had cases where the generators were copied
(instead of referenced).  I haven't traced when this happened, yet.
The class generator_reference_t provides a reference wrapper which
did prevent the copies in my experiments.

>   - Why is the triangle distribution undocumented?

Oversight. Consider it done.  Does anyone have a real-world
usage example?

>   - I would not restrict uniform_sphere on dimensions > 1

Ok.

>   - I think your implementation does not work if someone decides, e.g.
>     to use Rational Class as Real numbers.
>  Probably, you should assert
>     std::numeric_limits<base_result>::is_specialized.

This requirement is clearly stated in random-concepts.html, and
I can add this assertion nearly everywhere.  I don't think it's
worth the effort: Any reasonable programmer will specialize
std::numeric_limits<> for his own number classes.

>  Exception safety
>     would also be unclear, if types like IntType and RealType were
>     classes.

Very unclear, i.e. not yet checked for at all.  In particular, the
seed() and operator() functions must be checked.  Problems will
arise with the generators such as mersenne_twister.  They have
large internal state, which cannot be handled atomically.  Fixing
this requires too much changes for now, I think.

>   - Probably a compile time assertion should be a boost-library.

We had this topic a few months ago and it died away.  There
does not seem to be a universal solution.

Jens Maurer