Re: [boost] [Review] Review of the Accumulators library begins today Jan 29

From: Steven Watanabe (steven_at_[hidden])
Date: 2007-01-29 19:12:09

AMDG

John Phillips wrote:
> The formal review of the Boost.Accumulators library, submitted by Eric 
> Neibler begins today and ends on Wednesday, February 7. The library is 
> available from
>
> http://boost-consulting.com/vault/index.php?directory=Math%20-%20Numerics
>
>  From the documentation:
>
>   Boost.Accumulators is both a library for incremental statistical 
> computation  as well as an extensible framework for incremental 
> calculation in general.  The library deals primarily with the concept of 
> an accumulator,  which is a primitive computational entity that accepts 
> data one sample at a  time and maintains some internal state. These 
> accumulators may offload some  of their computations on other 
> accumulators, on which they depend. Accumulators  are grouped within an 
> accumulator set. Boost.Accumulators  resolves the inter-dependencies 
> between accumulators in a set and ensures that  accumulators are 
> processed in the proper order.
>
> Your comments may be brief or lengthy. If you identify  problems along 
> the way, please note if they are minor, serious, or  showstoppers.
>
> Here are some questions you might want to answer in your review:
> 	? 	What is your evaluation of the design?
> 	? 	What is your evaluation of the implementation?
> 	? 	What is your evaluation of the documentation?
> 	? 	What is your evaluation of the potential usefulness of 		 
>       the  library?
> 	? 	Did you try to use the library?  With what compiler?         		Did 
> you have any problems?
> 	? 	How much effort did you put into your evaluation? A 	 		glance? A 
> quick  reading? In-depth study?
> 	? 	Are you knowledgeable about the problem domain?
>
> And finally, every review should answer this question:
> 	? 	Do you think the library should be accepted as a Boost   		library?  
> Be sure to say this explicitly so that your  		 		other  comments don't 
> obscure your overall opinion.
>
> All interested parties are encouraged to submit a review. These can be 
> sent to the developer list, the user list, or if you don't want to share 
> your review with the general public it can be sent directly to me.
>
> Thanks to all for the work you will do on this review.
>
>
> 			John Phillips
> 			Review Manager
user_s_guide.html#id450311 line 363
accumulators.qbk line 189
for some reason external<> is not a link in the html

accumulators.qbk lines 35-37
[def _mpl_                   [@../../libs/mpl MPL]]
[def _mpl_lambda_expression_ 
[@../../libs/mpl/doc/refmanual/lambda-expression.html MPL Lambda 
Expression]]
[def _parameters_            [@../../libs/parameters Boost.Parameters]]
these links are bad

post_construct()
Unless for some obscure reason that I am missing it is just
too difficult, I would strongly prefer to make sure the constructors
are called in the correct order.

The whole dance with fusion::vector is unnecessary.
It would be simpler to use
    struct accumulator_tuple_base {
        template<class Args>
        accumulator_tuple(const Args&) {}
    }
    template<class T, class Base>
    struct accumulator_tuple : T, Base {
        template<class Args>
        accumulator_tuple(const Args& args) : T(args), Base(args) {}
    };
Since every accumulator is put before all the
accumulators that depend on it in make_accumulator_tuple
you can write
    typedef
        typename mpl::reverse_fold<
            sorted_accumulator_vector
          , accumulator_tuple_base
          , accumulator_tuple<mpl::_2, mpl::_1>
        >::type
    type;
Then every accumulator is constructed after any
others that it depends on. As a bonus the only
limit to the number of accumulators is whatever
the compiler can handle.

What happens if you freeze a feature that another feature depends on?
    #include <iostream>
    #include <boost/accumulators/accumulators.hpp>
    #include <boost/accumulators/statistics/mean.hpp>
    #include <boost/accumulators/statistics/sum.hpp>

    using namespace boost::accumulators;

    int main() {
        accumulator_set<double, features<tag::mean, droppable<tag::sum> 
 > > accum_set;
        accum_set(1.0);
        accum_set(2.0);
        accum_set.drop<tag::sum>();
        accum_set(3.0);
        std::cout << mean(accum_set) << std::endl;  //prints 1
    }
This behavior can easily cause surprises. It must
be fixed to print 2.  I'm not quite sure how to implement
it correctly. I think that probably you need to store the
result at the point that drop is called for external lookup
and continue to collect data for internal usage. Of course,
this only applies in the face of dependencies.

The user guide needs to specify what header each component is in.

BOOST_PARAMETER_NESTED_KEYWORD.html
There is no description of what this macro is for.

accumulator_set.hpp line 149
corresponds to -- What?

accumulators_fwd.hpp #define's
FUSION_MAX_VECTOR_SIZE/FUSION_MAX_TUPLE_SIZE
This is a very bad idea as it might make
make the value smaller than it would otherwise
have been. Please just check FUSION_MAX_VECTOR_SIZE
and issue a #error directive if it is too small.

For simple cases at least the library seems
to cause little to no overhead with msvc compared to
hard coding everything.

The docs need to state clearly what happens when
more than one accumulator maps to the same feature.

Overall the users guide is quite good.
I had no difficulty understanding it.
The reference manual is pretty sketchy though.

For the most part the interface is clean and easy to use.

I definitely think Accumulators should be included in boost.

In Christ,
Steven Watanabe