Re: [boost] [histogram] Formal review

Dear Bjørn,

On 23. Sep 2018, at 16:26, Bjorn Reese via Boost wrote:

Boost.Histogram contains useful and well-designed features, like excess (over/underflow) bins, and axis transforms to name a few.

As this is a review, I am going to spent most time below on critical scrutiny. […]

thank you very much for the great review and the time spend with the library. Some discussion below.

I. DESIGN ---------

Iterators are const. This prevents us from bootstrapping the histogram with a prior distribution using STL algorithms like std::fill(), std::generate(), or std::sample().

Writable iterators could be added, but I am not convinced that is a good idea. The histogram iterators are const, because I cannot imagine a use-case where the histogram has to be bootstrapped with a prior distribution. Only providing const iterators avoids the misconception that a histogram is a container. Histogram can make stronger guarantees about its internal consistency, if users cannot set bin counters arbitrarily.

The adaptive_storage does not work well with STL algorithms, because weight_counter is not an arithmetic type. See the Implementation section below for more detail. I therefore propose that the default storage policy is changed to something without weight_counter.

There are two solutions, right? Turing weight_counter into a full arithmetic type or removing support for weight_counter in the default storage.

The adaptive_storage has two responsibilities: data compaction and weights. Would it be possible to split this into two separate storage policies? I have no real use for arrival variance.

Agreed, it should be a compile-time option for adaptive_storage. https://github.com/HDembinski/histogram/issues/75

I am not too fond of using operator() for insertion. The code looks like a function call with side-effect. I prefer an explicitly named member function.

Originally, I had a special method called fill(), then I switched to operator(). There are two arguments in favour of operator(): - a 1D histogram can be filled with std::for_each or std::accumulate - consistency with Boost.Accumulators, which also use operator()

The axis::ouflow_type is oddly named. I suggest that this is renamed to something like axis::excess_type.

I am ok with renaming, but I think we haven't found the best name yet. What do you think about axis::tails_type?

II. IMPLEMENTATION ————————— […]

Agreed, the missing bits will be implemented and unit-tested.

III. DOCUMENTATION ————————— […]

Agreed, documentation will be expanded in this way.

Boost.Histogram should be ACCEPTED into Boost, provided:

1. Reference documentation is finalized.

I furthermore strongly recommend that the default storage policy is changed to something without weight_counter because of the various problems with STL algorithms. This recommendation is not a prerequisite for acceptance.

Thanks!