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@lists.sourceforge.net? -- Eric Niebler Boost Consulting www.boost-consulting.com