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. 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. 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. 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.)
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? ;-) )
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.) 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<>.
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?
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 also wondered why you always write
For implementation details, see foo_impl. and "hide" the formulas there?
I think the statistical accumulators reference docs can be greatly improved with a better structuring. 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... Greetings, Hans