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

From: Eric Niebler (eric_at_[hidden])
Date: 2007-01-31 13:27:05

Steven Watanabe wrote:
> AMDG
> 
> Eric Niebler wrote:
>> Hi, Steven,
>>
>> Steven Watanabe wrote:
>>   
>>> 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
>>>     
>>
>> Thanks. I think I was overly-optimistic when I wrote that. The links 
>> would work if Accumulators were in it's rightful (a-hem) place in the 
>> Boost tree. ;-)
>>   
> unless I missed something
> libs/accumulators/doc/../../libs/ == libs/libs/
> Which I think is not what you intend.  Somehow in the user_s_guide.html this
> becomes libs/accumulators/libs/ I'm not quite sure whether
> this is a quickbook bug or the file was built as though it were in
> libs/accumulators/doc/html/

I think it's correct if the docs are built from the top-level 
$BOOST_ROOT/doc directory.

>>   
>>> 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.
>>>
<snip>

>>
>> This would be a valid design choice, but it would involve reinventing a 
>> lot of Fusion. I use Fusion both to generate the accumulator set, to 
>> process the accumulators, and to find accumulators that match Fusion 
>> predicates. Code reuse is good.
>>   
> I'm not suggesting that you reinvent the
> fusion algorithms.  I just think that fusion::vector
> is not necessarily a good sequence to use
> when the constructor parameters are all identical.
> Also if you write the sequence then you can
> guarantee the order of initialization.
> 
<snip>
>> Regarding post_process(), I decided at the time, after discussing this 
>> with Joel de Guzman (of Fusion) and Dave Abrahams, that relying on the 
>> construction order of accumulators within a set would be a mistake. It 
>> would rule out size optimizations like compressed_pair, where empty 
>> accumulators are optimized away via the Empty Base Optimization. Fusion 
>> doesn't do this optimization today, but it may in the future.
>>   
> But EBCO can't help you anyway.  Two distinct sub objects
> of the same type (accumulator_base) cannot share the same address.
> I assumed that you didn't want to rely on fusion initializing the elements
> in order, that was part of the reason I suggested rolling your own fusion
> sequence.

Hmm, you're right. post_construct should go away.

>>> 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 code is doing something a little unusual, but the result looks 
>> correct to me. What do you think the result should be, and why?
>>   
> The result should be 2 because 1.0 + 2.0 + 3.0 / 3 = 2.0
> Users of particular accumulators should not have to care about the
> internal dependencies. The mean should return the mean of all the
> values that have been added regardless of whether count/sum have
> been dropped.

OK.

>>> 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.
>>>     
>>
>> I had it that way at first, but changed it due to user feedback. The 
>> idea is to make it so that you can just #define 
>> BOOST_ACCUMULATORS_MAX_FEATURES and not have to worry about configuring 
>> Fusion.
>>
>>
>>   
> 
> Consider.
> 
> file a.cpp
>     #include <boost/accumulators/accumulators.hpp>
>     #include <boost/fusion/sequence/container/vector.hpp>
> file b.cpp
>     #include <boost/fusion/sequence/container/vector.hpp>
>     #include <boost/accumulators/accumulators.hpp>
> 
> I'm not absolutely certain that this actually causes a problem
> with ODR, but even if it doesn't now it is very fragile.

Yikes. OK, this problem goes away if I abandon fusion::vector in favor 
of an open-ended Fusion sequence that guarantees construction order. I 
see Fusion cons<> is such a sequence. I might just switch to that and 
kill 2 birds with one stone.

>>> The docs need to state clearly what happens when
>>> more than one accumulator maps to the same feature.
>>>     
>>
>> Right. The first accumulator with a particular feature is the one that 
>> gets used. Any others with the same feature are ignored.
>>   
> And the ones that are specified in the sequence are processed before those
> found indirectly.

Exactly. Thanks for your valuable feedback.

-- 
Eric Niebler
Boost Consulting
www.boost-consulting.com