Re: [boost] Accumulators library documentation review

From: Eric Niebler (eric_at_[hidden])
Date: 2007-02-07 15:26:42

Hans Meine wrote:
> Hi Eric, John,
> 
> I wanted to send a full review for the library, but it looks as if I need a 
> boost CVS checkout for that (for boost/typeof/typeof.hpp), and I am very 
> short on time right now.

Yes, sorry. It works with the RC of 1.34 and with CVS HEAD.

> So I at least send what I already have: Lots of comments on the documentation:
> 
> Comments on the Documentation 
> =============================
> 
> (Disclaimer: In some of the following, I purposely pretended to be
> dumber than I am, in order to show where the explanations could use
> some improvements. ;-) )
> 
> In the "Passing Optional Parameters", I read about tail and
> tail_variate.  Not being an expert in statistics, I had problems
> understanding the example, so I tried to find out what tail_variate<>
> does.  Unfortunately, I could only find out that it takes three
> template parameters (VariateType, VariateTag, LeftRight), but I could
> not even see what they mean.  LeftRight is passed to tail<>, and then
> to tail_cache_size_named_arg<>, but I still do not see what the
> left/right is for.  -> Update: I found the answers in the Statistical
> Accumulators Library's documentation.  It would be much better AFAICS
> if the links of tail<> and tail_variate<> would point to that
> appropriate documentation.

Noted.

> Having said that, I think it could be useful to include links to
> external sites explaining some of the statistics terms, if you know
> such.  I bet there are more people like me, who have only little more
> than basic knowledge in statistics but who are interested in the more
> complicated features once they start using your library.

It would be nice to include a link to a good online statistics 
reference. Can anybody recommend one?

> A comment in a code example reads:
>> All accumulators should inherit from accumulator_base.
> Add a small note somewhere why this is necessary / what is inherited?
> Also does "should" mean that this is mandatory?  (Update: OK, now I
> saw the note later down in the docs about the default operator().
> Still I don't know if that's all.)

I'll fix that.

> Then you wrote:
>> Although not necessary, it can be a good idea to put your accumulator
>> implementations in the boost::accumulators::impl namespace. This
>> namespace pulls in any operators defined in the
>> boost::numeric::operators namespace with a using directive. The
>> Numeric Operators Sub-Library defines some additional overloads that
>> will make your accumulators work with all sorts of data types.
> It does not look clean to me to put my own stuff in someone else's
> namespaces; is there a problem with just "using namespace
> numeric::operators;"?  (OK, which? ;-) )

Yeah, that's a bit untidy. I should change the recommendation to putting 
your accumulator implementations into your own impl namespace and 
putting a "using namespace boost::numeric::operators;" directive in your 
namespace.

> Documentation of the Accumulator Concept: Maybe link back to "Optional
> Accumulator Member Functions" for post_construct and on_drop?  (As it
> stands, one can understand the concept definition only after reading
> much of the documentation before, although one might want to quickly
> check the concepts - possibly when coming from the first reference to
> the concepts in the docs, which is before the explanations.)

Good suggestion.

> In the Feature Concept, I cannot understand the explanation of
> F::is_weight_accumulator - it seems to me as if a second occurence of
> S (template parameter?) is missing somewhere?
> 
> Ah, now I see:
>> The weight accumulators are made external if the weight type is
>> specified using the external<> template.
> Still, that part of the API is missing an example, isn't it?
> I have difficulties understanding when and how to use external<>.

Yes, John already noted that I was missing an example of using an 
external weight accumulator. That should make it clear(er).

> At the end, just above the TODO, there are two items:
>> * Mapping multiple impls to the same feature with feature_of 
>> * Creating aliases for features with as_feature
> I guess these are also placeholders for future doc snippets?

Whoops. Yes, that's a TODO list that never got fleshed out. Thanks for 
spotting that.

> The Statistical Accumulators Library
> ====================================
> 
> I was very glad to find formulas and article references for
> non-trivial accumulators like the p^2 median estimators.  How about
> sth. like that for more of the accumulators (e.g. "kurtosis", or even
> for mean and variants like immediate_mean)?

I'll see if I can convince someone who knows LaTeX to write some more 
formulas for me.

> I also wondered why you always write
>> For implementation details, see foo_impl.
> and "hide" the formulas there?

That's due to some shortcomings in our documentation tool chain. It's 
pretty easy to get doxygen to generate the formulas from LaTeX embedded 
in C++ comments. Anything else involves hacking LaTeX and our tool 
chain. I know that's a lame answer, sorry. :-P

> I think the statistical accumulators reference docs can be greatly improved 
> with a better structuring.

As I mention in a previous reply, I use Boost's Doxygen/BoostBook tool 
chain to generate the reference section. I'd hate to abandon that 
approach, because it gives all Boost docs a uniform structure. It so 
happens that the structure is uniformly /bad/, and making it good would 
take a lot of work. Not sure what to do about that.

> Also, I very much dislike the look in my browser (Konqueror), but I guess it 
> may be the fault of some "standard boost CSS".
> Looking into it right now, it seems that since "The Statistical Accumulators 
> Library" is a subsection of the user's guide, all sections inside it become 
> either <h3> or <h4> and thus nearly-indistinguishably small (hardly larger 
> font sizes than the usual text).  That should be probably discussed in a 
> separate thread to get some attention by the responsible...

That sounds like a bug in boost.css. Could you report it to 
boost-docs_at_[hidden]?

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