TClonesArrays for the 21st century: Syntax, type safety and lookup

I suspect that one of the reasons why I see so many FairTasks doing unrelated things (e.g. god class antipattern) in a project using FairRoot is that writing a typical FairTask includes lots of boilerplate code.

What I observe is the following:

For each event data set used, a member TCA* is declared in the class.
In every constructor, that member is then set to NULL (or nullptr) using member initializer lists ('if ROOT did not have default member initializers in the 1990s, we sure as heck don't need them either')
In the Init() method, the TCA pointer is then assigned to the return value of FairRootManager::Instance()->Get(...) (or registered for new output TCAs)
Finally, in Exec(), the elements of the TCA are accessed by index and cast into the actual data type using C-style casts (because we really like the 90s, apparently).
In the destructor, the TCA* received by FairRootManager::Get is then deleted ('there is no such thing as too many deletes')

I feel that modern C++ offers some potential to improve on this procedure. Recent C++ offers a feature called "templates" (which were shunned by ROOT devs for decades because ROOT5's C interpreter did not understand them) which has been used with container classes to some success (see e.g. the STL). Exposing users to raw type-unsafe ROOT containers such as TClonesArrays should be no more acceptable than exposing them to void* or exposing kindergardeners to unshielded live wires.

Different people have at different times tried their hand at insulating users from TCAs, the approaches I know of are @janmayer's TCAConnector and my own roothacks::typed_collection. Both of these use templates to achieve type safety and give users the ability to use modern range based for loops.

I do not understand the rationale behind the separate Init() method. (Same also goes to SetParContainers()). I guess it is probably related to the ability to do ReInit(), but I don't understand that either. IMHO, it would be acceptable to have one FairTask doing its thing for its lifetime. If the user has to analyze other data with different parameters, have them instantiate another object (preferably in another process). I think it would not be too much to ask users that FairRun are instantiated and configured and that the parameter files are loaded by the time they instantiate their tasks, and that they instantiate their tasks in order, so that no task uses inputs by a task not yet created. If these were the case, all of initializations could be taken care of in the constructor.

If one does not change the multi-pass initialization, then one could still try calling FairRootManager::Get() when the TCA is used for the first time (probably in Exec()), as I do in roothacks::DContainer. For TCAs created by a task, I have no good workaround to avoid Init(), presumably, they have to be Register()ed in Init because other FairTasks will try to use Get() on them within their Init methods. (An abstract template <typename T> class TCAWriter: public FairTask, which overloads Init() would work in theory, but be somewhat fragile. Or FairTask could simply have a std::vector<std::tuple<TClonesArray*, std::string>> member whose content it registers during InitTask(). Or one could call InitTask from FairRun::AddTask already, thus making sure that all tasks are fully constructed, and then just Register them in the constructor.)

I think these things could reduce the amount of boilerplate associated with handling TCAs drastically. (To preempt the argument that "users will not understand templates": as of now, users have not balked at the use of placement new when creating TCA elements, a feature of C++ which I think is way more obscure than templates. If they can copy-paste "new (clref[size]) Foo(...)", they surely can also copy-paste TCAInputConnector<Foo> or the like.Of course, encapsulating the placement new would be a really good application of variadic templates.)

Now for something completely different:

For many detectors, the array-esque container provided by TCAs is not a good fit for the data. Take a silicon strip detector whose strips are read out on both ends. During the calibration step (what would be called Mapped2Cal in R3BRoot), one will likely want to find the complementary side of the hit currently being processed. Because TCAs are not associative, one can not directly look it up by channel number. Instead, one either has to prefill a std::map<int, Foo*> or use the more naive approach of searching the TCA in linear time per entry for a total processing time of O(n^2). If, in the next step, one is interested in inter-strip events, one is again stuck with recreating the std::map which allows efficient lookup to detect if channels n-1 or n+1 were also hit.

Of course, ROOT's own reinvention of the wheel -- TMap as their answer to std::map -- lacks templates and requires a TObject* as a key instead, which is obviously not very efficient even if there is some TObjInteger hidden somewhere.

A persistent storage of associative information for a TClonesArray object within FairRootManager obviously can't depend on the type of elements. For backwards compatibility, one could keep TClonesArray, but complement it with a std::map<int, TObject*> pointing to the TCA elements.

A user would then access it using a templated class which knows about his data type:

template<typename T>
AssociativeTCAInputConnector // needs catchier name
{
    AssociativeTCAInputConnector(std::string name)
    {
         auto frm=FairRootManager::Instance(); assert(frm);
         this->fTCA=frm->Get(name);
         this->fMap=frm->GetMap(name);
    }
    T& operator[](int n)
    {
         return dynamic_cast<T&> (this->fMap->at(n));
    }
    // begin(), end() as in roothacks::DContainer or whatever. 

protected:
     TClonesArray* fTCA;
     std::map<int, TObject*>* fMap;
};

Another class (AssociativeTCAOutputConnector?) would register both the TCA and the map and provide

template<typename ... Args>
T& Add(int n, Args... args)
{
    auto res= new (*this->fTCA)[this->fTCA->GetEntries()] T(n, args...);
    assert(!this->fMap->count(n) && "duplicate index");
    *(this->fMap)[n]=res;
    return *res;
}

Which both handles both the insertion in the TCA and the map. (Code samples are untested and meant % bugs.)

That way, one could add index-based lookups where appropriate without disrupting any code which relies on the data being in a TClonesArray.

I think that this "manual" setting TCA to nullptr was related with PROOF (as far as I remember this was even mentioned in ROOT manual to set pointers to nullptr). I also think that problem of double deallocation is related not to TCA but to any data passed to/from FairRootManager. Concerning Init - I think is a useful method to check if everything is correct with data/settings before you can "do" some stuff in exec - not everything can be done in constructor or setters.

   return dynamic_cast<T&> (this->fMap->at(n));

:nerd_face: adventurous, but fine with me. However, you should not unconditionally dereference fMap.

I do not understand the rationale behind the separate Init() method.

AFAICS, it is a two-fold answer:

Scheduling/timing - the framework controls when initialization happens
There is a return value indicating success/failure which is actually a standard case when people resort to two-phase construction if they do not want to use exceptions.

I feel that modern C++ offers some potential to improve on this procedure.

I agree and I am personally also frustrated by the degree of technical debt we have in FairRoot, but also ROOT and other projects in our community. Your post touches a lot of topics and I am afraid I cannot go into details for a proper discussion of those topics (due to lack of time and/or answers).

However, your unhappiness about TCA is shared by me and by others. Some of them (not me, before my time here) have therefor developed the possibility to use std::vector<T*> and replace TCA completely. Check the STL example template and the FairRootManager::RegisterAny() interface. I am personally not too familiar with all this, but I wanted to have it mentioned for your consideration.

I am personally also frustrated by the degree of technical debt we have in FairRoot, but also ROOT and other projects in our community.

I look back at my years of learning C++ and getting into ROOT with some distance now, having left almost a year ago.

And I have to say: Its funny to see how insane everything the whole thing really is.

We require students to learn not only physics, but also several programming languages, including one of the most complicated and legacy-infested ones that even professionals cannot get right all the time.

Now, the best thing: It still works most of the time! Now imagine that instead of fixing memory leaks in a framework that is older than you, all this amazing brain power could be used using state-of-the-art data pipelines and systems!

At this point, I am convinced that ROOT is the one thing which is holding back nuclear and particle physics the most.

(Mostly not-precisely-on-topic)

@janmayer: I think ROOT is going on from sheer inertia. After a decade of working with it, people start to consider it normal and healthy that:

TH2 is a public TH1 (Liskov who?)
You have to save and restore global pointers (gDirectory, gPad) you implicitly modify to avoid breaking surrounding code
SetOptStat(11111) is a fine way to pass arguments
Drawn coordinate systems are an intrinsic property of histograms to the point that TGraph and TF1 have a method GetHistogram, while the log-ness of an axis is the property of the TPad it is drawn on
Cling will spam you with tons of unrelated "Unknown type" errors if you e.g. try to instantiate an abstract or misspelled type (Privately, I cope with this by considering CINT/Cling abominations unto all that is good and these errors are the cosmic retribution for people too lazy (including, sadly, myself sometimes) to compile their code like our foreparents did. Okay, so perhaps I am not quite in the stage of ROOT-acceptance yet.)

I think teaching students C++ by having them use ROOT is like teaching kids swimming in the a stormy, shark-infested ocean: their experience will not be all that great and even the survivors are unlikely to pick up good technique, but instead learn mostly to rely on clinging to driftwood or copy-pasting code they don't really understand.

I think my biggest philosophical beef with ROOT is actually with what one might call an implied totalitarian attitude: Instead of being a library offering some tools the programmer can select or not (think boost or scipy), ROOT's approach to interoperability is loosely based on Exodus 20:02:

Whereas boost uses prefixes in library names and namespaces, ROOT just claims whatever it likes.
While other libraries are threadsafe and you can just use them with libpthread, ROOT requires you to use their encapsulation TThread.
While the rest of the world is using sin from math.h orcmath, ROOT prefers you to use TMath::Sin().
Where other software tries to implement open standards and have interoperable file formats, ROOT firmly sticks to their own to their proprietary formats (and leaves interoperability to third parties).
If other software like Geant4 becomes popular, it gets embraced and extended in ROOT.
Cling is a walled garden: you get what ROOT sees fit to provide you (which nowadays includes STL, amazingly) while any third party library will have to go through lots of trials and tribulations to attain the mark of ROOT which allows usage within Cling. I find these tendencies about as endearing in ROOT as I found them in the 1990s Microsoft.

@dennisklein: Thanks for the link to RegisterAny. I think I will try to implement my weird TCA+std::map<int,TObject*> combination based on that.

I have created a basic implementation of an (optionally associative), type-safe, interface for TClonesArrays with minimal fuss for the users. #1249 are the changes in the FairRoot codebase.

The proof of concept is here. (Links goes to a R3BRoot repository. Readers discretion is advised.)

typed_collections.hh contains the stuff which is not specific to FairRoot, while fair_collections.hh contains the FairRoot specific stuff. (Plus some R3BRoot specific stuff, for now.)

R3BCalifaCrystalCalData is a class meant for associative lookup. There exists a field which is different for all its instances belonging to one event:

    using keyT=int;
    keyT GetKey()
    {
      return fCrystalId;
    }

The key must be set correctly directly in the constructor.

R3BCalifaMapped2CrystalCal is a FairTask which reads from one TCA and writes another. The input TCA is not required to be associative, but the output one supports associative lookup:

class R3BCalifaMapped2CrystalCal : public FairTask
{
...
  roothacks::DContainer<R3BCalifaMappedData> fIn;
  roothacks::ADContainer<R3BCalifaCrystalCalData> fOut;
...
};

In the constructor, both of the containers are initialized:

R3BCalifaMapped2CrystalCal::R3BCalifaMapped2CrystalCal(const std::string in, const std::string out)
    : FairTask("R3B CALIFA Calibrator")
    , fIn(in)
    , fOut(out, this, 0)
{
}

fIn is for reading and will be initialized using FairRootManager::Get on first use, while fOut has to be registered during the init phase, which we will do with the trick shown in #1249. (For now, the second argument not being nullptr marks an output collection. This could probably be made more explicit.)

In the Exec method, fIn is used via a range based for loop:

for (auto& mapped: fIn)
{
 // calibration happens....
   auto& outHit=fOut.emplace(crystalId); // the primary key must be set in the constructor. 
   outHit.fEnergy=cal[en];
    // ...
}

As the result is now an associative container, another task can also use this for lookup:

      if (fCal.count(k1.GetKey() + ncg))
        {
          auto& proton = fCal[k1.GetKey() + ncg];

(I have not tested that this is working correctly for now, but I am convinced that it can be made to work without too much effort.)

As both roothacks headers show, just like with putting bathroom piping or high voltage conductors out of the view of the user, the fact that stuff is hidden from a user (at least until they flush the wrong type down and all the template parameters explode in their face) should not be confused with an exceptional beauty of the code/piping/wiring doing the actual work: no caterpillar function has ever been turned into a butterfly function by putting the template keyword in front of it.

Still, I think that from a Health & Safety perspective, providing (eventually) bug free interfaces to icky and dangerous stuff like mains power or TClonesArrays is a good idea.

At the moment, the roothacks namespace is not ready for inclusion in FairRoot. Nor am I very inclined to lobby very much for their inclusion. They are purely templated headers which will work for no matter where I put them. If you do consider including them, please tell me what to change (e.g. "No use of throw, all class names have to start with Fair, separate class for out collections, less whining about root in the comments") and I can make a PR.

Cheers, Philipp

FairRootGroup / FairRoot

TClonesArrays for the 21st century: Syntax, type safety and lookup #1225