StFMSPointMaker review

comments :

general infrastructure :
1) maybe create a StRoot/StFmsUtil where the following codes will be located :

StFmsClusterFinder.cxx ,   
StFmsClusterFitter.h, StFmsEventClusterer.h, StFmsGeometry.h,StFmsTower.cxx 
StFmsClusterFinder.h, StFmsFittedPhoton.h , StFmsTowerCluster.cxx, StFmsTower.h 
StFmsClusterFitter.cxx, StFmsEventClusterer.cxx, StFmsGeometry.cxx, StFmsTowerCluster.h    

from STAR doc.

StXXXUtil : Code compiled in a Util or Utilities tree should be code which do not perform any action (nor a maker) 
but constitute by itself a set of utility classes and functions. Other classes may depend on a Utility library.

2) doxygen documentation is really helpful
 -note : the doxygen seems outdated , eg : StFmsEventClusterer description is different from the RCF code. (but that's a really minor issue)

3) about TRef :

 When the TRef is read, its pointer fPID is set to the value
 stored in the TObjArray of TFile::fProcessIDs (fProcessIDs[pidf]).
 The pidf is stored as a UShort_t limiting a file to 65535 distinct
 ProcessID objects.
 The pidf is stored in the bits 24->31 of the fUniqueID of the TRef.
 This implies that the number of TRefs in a single ProcessID should not
 exceed 2**24 = 16777216.


- does this mean that one file could have 65535 dfferents PiD's, in the FMS case, 65535 events analyzed ? (I guess it shoudl be okay)
- 16777216 is the number of references , I guess this is the number of objects that refer to a given Pid. Unless you save that number of clusters/hits, it should be okay too.
- if the Pid is unique, it also mean that other STAR codes can use TRef, as long the number of Pids is not > 65535

Details :

StFmsPointMaker.cxx
- line 45 (InitRun) : you access StFmsDbMaker to init the geometry, then you initialize the geometry.
It seems to me that you may do everything in StFmsDbMaker, in that case you remove one step and you may also remove the StFmsGeometry class
actual : StFmsPointMaker --> StFmsDbMaker --> initialize geometry.
better (?) : StFmsDbMaker --> initialize geometry

- line 67 (Make) : there are 2 methods : populateTowerLists() and clusterEvent(), but both are checking for a FmsCollection(). If the one in populateTowerLists() fails, the event is aborted so there is no need of redoing the check in clusterEvent()

- line 240 (isValidChannel) : hard-coded numbers for rows and columns

- line 270 : mFmsDbMaker is verified here but it has alredy been verified in the Init() and if there is no DbMaker, the chain returns a kStErr so there may be no need to double check

- line 129 : validateTowerEnergySum() : hard-coded centerOfMassEnergy

- line 148 : processTowerCluster() :there is a Id coding using hard-values
 cluster->setId(305 + 20 * detectorId + fmsCollection->numberOfPoints());

StFmsEventClusterer.cxx :
- methods OnePhotonFitParameters(), twoPhotonsFitParameters(), globalPhotonFitParameters() seem to have also constants/hard-coded cuts. Doc ? Do they depend on a given energy , run ?
Maybe define a StFmsConstant where you set these cuts.

- line 243 : towerSize hard-coded to 578

- line 251 (fitEvent) : fitClusters() method calls fit1PhotonCluster,  fit2PhotonCluster and fitAmbiguousCluster. These 3 functions are using TMinuit fit ; does it take time to do this (real and CPU) ? CPU usage ?

- line 288 (fitClusters) : this method calls StFmsClusteFinder::categorise. In this method, "empirical" criterias are used --> i) hard cuts/values ii) Can these cuts depend on the collision energy ?

- line 325 (FitGlobalClusters) : this function is used as fitGlobalClusters(nPhotons, mClusters.size(), mClusters.begin()); but the definition of the function does not specify explicitly the format of the 2 first inputs :

Double_t StFmsEventClusterer::fitGlobalClusters(unsigned nPhotons,const unsigned nClusters,ClusterIter first) {

I would have put :
Double_t StFmsEventClusterer::fitGlobalClusters(unsigned int nPhotons,const unsigned int nClusters,ClusterIter first) {

StFmsClusterFinder.cxx :
- line 42 : cut on tower energy is using hard cut (1.6*hit->energy)

- line 72 (towerEnergyIsAboveThreshold)  : cut on low energy is 0.01 (hard cut)

Naive question : why associate these towers again ? The Dyoxygen doc. says in order to prevent " bogus peaks". For my knowledge, could you expand a bit more ?

StFmsTowerCluster.cxx :
- line 34 : any reason why mThetaAxis is initialized to -10 ? Is this a particular value ?