Add 2D constructor for Histogram to allow construction with make_unique etc

The current generic Histogram constructor Histogram(OWNER*, string, string, initializer_list<Axis>) isn't recognised by make_unique, etc so it's not currently possible to create a smart pointer to a >1D histogram (1D histograms having an alternative constructor already):

../GaudiKernel/tests/src/CounterHistosUnitTest.cpp:135:15: error: no matching function for call to 'make_unique'
  auto hist = std::make_unique<Histogram<2, atomicity::full, float>>(&algo, "Test2DHist", "Test 2D histogram", {{10, 0., 10.}, {10, 0., 10.}});
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This MR adds a 2D constructor analogous to the existing 1D case and a test to show that it can be used to create a unique_ptr.

mentioned in merge request lhcb/Allen!641 (closed)

This is too specific to 2D histograms. If you would really like to go that direction, you would need variadic templates to make it work for all dimensions.

Dropping the previous change to Histogram.h and rewriting this :

  std::initializer_list<Axis<float>> axis = {{10, 0., 10.}, {10, 0., 10.}};
  auto hist = std::make_unique<Histogram<2, atomicity::full, float>> (&algo, "Test2DHist", "Test 2D histogram", axis);

works. Details are given in https://stackoverflow.com/questions/26379311/calling-initializer-list-constructor-via-make-unique-make-shared

Now remains the question whether we prefer this extra syntax or an addition to histograms.h, similar to what you did but generic for nD. I would try to follow here recommendations of @graven that it should be hard to write bad code and easy to write good one, so avoid to facilitate the use of pointers and not change histograms.h. But keep this new test you added as the example on how to do it in case someone really needs it.

@clemenci @graven opinions ?

I would like to see the original problem that caused @dcraik to want to explicitly allocate histograms on the heap, and see whether that could be solved in another way... just in case it is an example of the XY-problem

Just for reference, my first answer (initially privately on mattermost) was :

I would rather try to switch to no pointers at all. If feasible of course, but it's very seldom not the case

@graven the original problem was that these histograms need to be members of a DeviceAlgorithm class within Allen but can't be instantiated in the constructor of the class (both because the axes may depend on properties of the DeviceAlgorithm that aren't available until later, and because the OWNER passed as the first argument of the histogram is this, which then results in a pure virtual method call if used in the constructor). A specific example is here:

https://gitlab.cern.ch/lhcb/Allen/-/blob/358caabca1bb540a1e32e5f22dc6c1750172b15f/device/selections/lines/calibration/include/D2KPiLine.cuh#L102 https://gitlab.cern.ch/lhcb/Allen/-/blob/358caabca1bb540a1e32e5f22dc6c1750172b15f/device/selections/lines/calibration/src/D2KPiLine.cu#L12

To update this, I'm still chasing down the pure virtual call issue but it seems to be a problem with the Entity that gets created rather than something showing up at construction time and I can't yet rule out the possibility of it being a peculiarity of how Allen uses those.

The more well-understood problem is that the histograms need to persist for the lifetime of the Algorithm class they are used within (so should be members) but many can't be constructed until initialisation time (the axis ranges are not known yet when the algorithm is constructed). These histograms therefore need some sort of wrapping and any call to the likes of emplace_back or make_unique is incompatible with the current ND histogram constructor.

The workaround @sponce suggests does fix this issue. However, from a user's perspective, a solution that doesn't require the float templating in more than one place would be more maintainable. I agree that a generic ND solution is the better option if such a solution exists without the same issue.

a pure virtual call is typically due to calling a virtual method in a base-class constructor, which happens to run before the derived class constructor (as is also clear from how the derived class constructor is run: it first initializes the base class, before then entering the body of its own constructor. Only once the derived class constructor actually constructs the concrete derived type is its vtable populated, and hence only at that point can a virtual function call be made.

This is now superseeded by !1273 (merged), which tackle all dimensions, not only 2D. It also uses an std::deque and emplace_back in the test rather than make_unique which should not be used that often

closed