Moved PrPixelTracking to functional

This involved actually a lot of code refactoring. In particular :

• PrPixelStoreClusters algo has been merged into PrPixelTracking (was always used just after it anyway)
• PrPixelMonitor algo has been merged into PrPixelTracking It is not activated by default, and the Property Monitor allows to activate it
• PrPixelHitManager tool was heavily changed in order to drop any state of it. This involved redoing completely the memory allocation, which was heavily optimized. So tests have to be performed to check the efficiency of the new code With this modification, PrPixelTracking is functional but not yet completely thread safe. 2 things are missing :
• thread safe histograms in the monitoring
• usage of derived conditions for the remaining state in the HitManager. These will change only when geometry changes, so almost never. But it still should be properly done

PrPixelStoreClusters algo has been merged into PrPixelTracking (was always used just after it anyway) Does it mean it is always executed? Is there an option to not execute it?

Right now, it is always executed indee, as it seems it was always the case. Now it's trivial to add a boolean property to be able to skip it. Do you see cases where we need this ?

Yes, my understanding is that PrPixelTracking decodes clusters internally. PrPixelStoreClusters is needed to create clusters in a format used by the track fit. If you do not fit the tracks, this is not needed. And it would artificially increase the run time of PrPixelTracking. @tnikodem @adudziak Can you confirm?

In the Fast Stage PrPixelStoreClusters is only needed if you want to run the KalmanFitter. I am not sure if some other algorithms afterwards try in some configuration to access Velo Clusters.

Validation started with lhcb-future#186

Added 1 commit:

• 3d9d2c7d - Moved PrPixelTracking to functional

previous commit drops modification to CMakeLists.txt and cmt/project.cmt that wre present in the commit by mistake. No change to the code

Added 3 commits:

• 37d26c56 - Moved TrackBestTrackCreator to functional
• 13385ffd - Added StoreClusters property allowing to disable the creation and storing of clusters
• 4b4344c8 - Fixes to PrPixelTracking after is was testing with Gaudi Hive

Validation started with lhcb-future#187

why not simply

for (auto& hit : pool) modulehits[hit.module()].addHit(&hit);

?

Indeed. I did convert a lot of code to new syntax, but obviously forgot some pieces...

I suggest to add final, i.e. class PrPixelModuleHits final {

why nt indeed. But is there a real added value ?

Anything that constrains things should help the compiler ;-) And it avoids someone inheriting from this class in future... or at least make him/her think twice before doing so.

Do we really want the overhead of a vtbl pointer + loss of implicit move in PrPixelHit? Can we drop the (virtual) destructor altogether?

(OK, there is little to move in this class, but the point is still valid ;-)

Of course we should drop the virtual destructor. I'm surprised this one survived my code refactoring... now dropped.

I suggest adding final here as well: class PrPixelHit final

Yes, nad before you come there, same in PrPixelTrack.

another one -- is this virtual d'tor really needed? (and make the class final ;-)

done

Why merge the monitoring into the reconstruction algorithm ? This seems a move in the wrong direction to me, we should be aiming to keep the reconstruction algorithms as streamlined as possible and (optionally) run the monitoring in a seperate sequence.

Also note, if you do keep it like this (which i think would be a mistake) you should use GaudiHistoAlg as your base class. GaudiTupleAlg has additional baggage associated to it, for tuples, you aren't using I believe.

Validation started with lhcb-future#188

Validation started with lhcb-future#189

Validation started with lhcb-future#190

@jonrob : the merging of the monitoring is exactly trying to stay streamlined ! First you do not need to run the monitoring, you can disable it (actually, it's even disabled by default) via a property called "Monitor". Now when you want to run it, you need some internal state of the tracking that is not stored in the TES. So if you run it as a separate algo, you either have a stateful algorithm (or tool, as it was the case), or you need to store more than before. And you will have to store, as you do not know from inside whether the monitoring algo will be run later or not. With this implementation, only the minimal is stored and you are optimized in all cases. Concerning the base class, I'm not yet completely at easy with all of them, so you are most probably right. I'll fix that next week when I come back.

OK, I am still not completely convinced that this isn't setting a precedence of moving the monitoring into the reconstruction algorithms, which as a general principle I do not like. I will let it drop for now, but I think if we start to see a mass migration of the monitoring from what we currently run in the Moni or Check sequences in brunel, into the algorithms themselves, its something that will need discussing.

A few other small suggestions.

• You don't need to add your m_monitor flag, as the GaudiHistoAlg base class already has a property for this, which you should just reuse via the produceHisto() method.

http://lhcb-release-area.web.cern.ch/LHCb-release-area/DOC/davinci/releases/latest/doxygen/d3/d8c/class_gaudi_histos.html

By default this is on for algorithms using this base clasx, but you change this by running

setProperty( "HistoProduce",   false );

in your constructor.

• Also, at the point where you switch in the monitoring, you could use the compiler hint UNLIKELY (Defined in Gaudi somewhere) such that the optimiser knows to optimise the if statement for not being run.

if ( UNLIKELY( produceHisto()  ) )
{
 // do stuff
}

Chris