Re: [boost] [histogram] review part 2 (implementation)

AMDG On 09/25/2018 01:34 PM, Hans Dembinski wrote:

...

128: if (Archive::is_loading::value) { this->~variable(); } Don't call the destructor like this.

How should I call it then?

Don't. You should only call the destructor explicitly if you intend to end the object's lifetime. In particular, it should only be used for objects that were constructed with placement new. The code has undefined behavior, because you are manipulating an object that does not exist (because it has been destroyed). Even if you ignore that problem, it's not exception-safe.

...

- serialize for user defined axis types seems error prone, because labelled_base::serialize is inherited. If the user forgets to add serialize, the code will compile and silently slice the object.

How can I avoid this?

The only simple solution is: don't use inheritance. It's also probably possible to use the non-member version of serialize, somehow. Note: The same issue appears for other members as well, but I don't really consider it a problem, because they aren't optional, and are, therefore, less prone to being forgotten than serialize.

...

weight.hpp:

12: namespace detail { Be careful about exposing types from namespace detail in any way. It allows ADL in client code to look inside your detail namespace.

So better make weight_type and sample_type public?

Or in their own namespace.

...

axis/base.hpp:

- I only see operator==, but not operator!=.

Axes types are not required to have operator!=.

That doesn't prevent you providing it, and it is more friendly to do so.

...

- Actually, I find interval_view somewhat unconvincing. Why can't it just be a pure value that stores the upper and lower bounds? The value_type is usually going to be a built-in type anyway, not to mention that you haven't documented interval_view sufficiently for it to be used by user-defined axes.

Let's say an interval is returned as a std::pair. Every time you call axis[k], axis::lower(k) and axis::lower(k+1) are called to fill both values of the pair.

Now assume you want to fill a std::vector with all the bin edges (a realistic use case). You iterate over the axis, get the intervals p, and append p.first to the vector. When you reach the last bin, you also append p.second. All the other p.seconds are filled while iterating over the axis, but not used. For N bins, lower() is called 2 N times while only N+1 times the value is actually used. This is very inefficient.

interval_view was my solution to avoid this superfluous calling of lower(). Is the compiler is smart enough to avoid the superfluous filling of values that are not used? I don't know, maybe. If so then I would also prefer the simpler solution.

It should be possible. I would certainly assume, unless and until proven otherwise, that the compiler can optimize it as long as no reference or copy of the interval_view escapes from the immediate context.

Another nice aspect of interval_view: it is explicitly convertible to the bin index. This allows you to run a range-based for-loop over an axis and query the histogram directly with these instances

for (auto bin : histogram.axis(0)) { const auto value = histogram.at(bin); // only works because "bin" knows index }

That doesn't strictly require you to maintain a reference to the axis.

...

- Is there a reason that you're not just using std::vector for variable and category?

Yes, sizeof(detail::buffer<T>) < sizeof(std::vector<T>) and axis types should be as small as possible. We don't need growth, so there no need to store a capacity in addition to a size.

Why do the axis types need to be small? Even if you waste a bit of memory, it should be insignificant compared to the actual data, right? Also, if you're that concerned about memory usage, the label costs at least as much.

...

detail/axes.hpp:

521: optional_index call_impl(Tag, const std::tuple<T>& axes, It seems that this specialization prevents using a single element container with a single element static histogram, which seems inconsistent with the documentation and the behavior for dynamic histograms.

Correct, because this case is ambiguous. Let's say you have a 1d-histogram h1 and 2d-histogram h2. Both are dynamic histograms and you fill them.

h2(1, 2); // ok h2(std::make_tuple(1, 2)); // ok, one argument must represent two values h1(1); // ok h1(std::make_tuple(1)); // ambiguous!

h1 cannot know at compile-time whether it should unpack the tuple or pass it to the axis unchanged. Perhaps you have a weird axis that accepts tuples as value_type.

I had to make a decision here and the most reasonable one is to pass single arguments of 1d histograms unchanged to the axis.

Shouldn't the same reasoning apply to dynamic histograms as well? This definitely needs to be documented. The documentation says that you can pass a container, without saying that one argument is an exception. In Christ, Steven Watanabe