For convenience, the reviewers input. Justin in text below, Tom's in attached PDF files

 

StFgtDbMaker:

StFgtDbMaker/StFgtDbIdealImpl.h

-There are hard coded values for pedestals, status, gains, etc. in this class, which I think are not necessary.  Presumably if one wants to use an ideal configuration for DB tables like ped/status/gain/etc they should be loaded in a set of "sim" flavored DB tables and accessed in the exact same way as the "ofl" tables used for real data, right?  Hard coded ideal calibrations like this are not to be used as I understand it.

-If the above suggestion is implemented what is the remaining functionality of this class, just to use the "Naive" mapping? 

Presumably this could go to the DB as well (I think another one of Thomas's suggestions), could this class be removed to unify DB access for ideal and real configurations?

 

StFgtDbMaker/StFgtDbImpl

-Thomas already mentioned, but I'll reemphasize that assert statements need to be removed.

 

StFgtDbMaker/StFgtDb

-Several constants in PchargeFraction, PstripGain, and RstripGain should be in DB or constants header.  Then again, where are these even used?  I couldn't find where they're used, but could be missing it.

 

StFgtDbMaker/StFgtDbMaker

-In SetFlavor, is the flavor for ideal tables really flavor="ideal"? 

Typically ideal tables used for simulation at STAR have flavor "sim", is there a reason for using a different name?

-Again, you need to remove assert statements.  Add an error message and return an error flag, but don't break the chain.

-One thing I've found useful for the calorimeters is when tables are retrieved to print a line with info on what table was retrieved, eg. for the BEMC the timestamp validity is given:

StEmcRawMaker:INFO  - loaded a new              bemcPed table with

beginTime 2011-06-21 00:00:00 and endTime 2011-12-20 00:00:00 This allows you to go look in the DB and confirm that the table you intended to be loaded was really used, or if there is a problem you can know which DB table to look for the problem in.  Just a suggestions, you don't need to add it but you may find it helpful when debugging, etc.

 

 

 

StFgtUtil/geometry/StFgtGeom

-I can understand the usefulness of some of these hard coded arrays such as Rstrip_Phi_Low, Rstrip_Phi_High, Phistrip_R_Low, and Phistrip_R_High which if I understand correctly just define the beginning and ends of strips based on a strip index which I would think are just fixed and should not change.

-However, other mappings may be better suited for a DB if its possible they could be changing from year to year or something.

 

 

 

StEvent/StFgt*:

-Aside from Thomas's comments/suggestions, in general all look reasonable -Some assert statements here as well to be removed -For StFgtPoint I'm also curious what this "key" variable will be used for and how it is assigned.  One other comment here, probably need a conditional in the constructor to provide an error if one tries to make a point from 2 R strips or 2 phi strips, currently it will just fail the assert.  Also would be good to propogate a position and error for the point similar to the hits (as commented as "TODO").

 

That's all,

Justin

-----------------second mail below--------------

 

Hi Anselm, All,

Still not done, but I'll send what I have for now.  I still owe comments on StFgtDbMaker, StFgtGeom, and StEvent/StFgt*, but that will probably have to wait until Tuesday (I'm flying to BNL tomorrow).  I briefly read through Thomas's very detailed comments as well, and tried not to repeat things where I hadn't already written the comment previously.  In general, I would say the code is in decent shape, but documentation is lacking.  At a minimum a short documentation can be added to each of the header files, similar to StFgtA2CMaker/StFgtA2CMaker.h.  First some specific comments on the makers, then some general comments at the end.

 

StFgtRawMaker:

-Change "PrepareEnvironment" -> "prepareEnvironment", "FillHits" -> "fillHits"

-What is the usefulness of "TStopwatch clock;" in Make()?

-In fillHits():

     -should masks of full rdo/arm/apv be hardcoded or better included in some DB masking table

        -it should probably at the very least have some timestamp flag associated with it so only 2012 data needs to satisfy these conditions

     -Strip type should not be hardcoded, as this flag is needed to separate ZS and non-ZS data.  How will this be implemented?  Maybe you could have separate data banks in the DAQ file, like the BSMD?

 

 

StFgtA2CMaker:

-remove printf statements like the following and replace with LOG_DEBUG #ifdef DEBUG

         printf("A2C for iDisc=%d\n",discIdx); #endif -I guess strip fitting over timebins is currently disabled, but will this be the planned mode of operation (I would think so from all the commissioning working on time bin fits by Len et. al.)?  ie. each strip that is satisfies some thresholds will be fit over timebins? If so, 2 more questions:

     -how many fits will this be per event in pp500 high luminosity?

     -is the cpu footprint still small relative to total event processing time (maybe its negligible, I don't know)?

-Several commented out "cout" statements should be removed or replaced with LOG_DEBUG

-Some lines of code eg: "//                        if( (mRelThres &&

adcMinusPed > mRelThres*pedErr) && ((mAbsThres<(-4096)) ||  adcMinusPed  > mAbsThres) )", are commented out and should either be removed or need a switch added if these are intended to be used for some special cases -General questions: Why is strip->setGeoId( -1 ); the default way to invalidate a strip?  Is there not some better way to flag it as not usable?

 

 

StFgtClusterMaker:

-It seems to me you should add the "default" cluster algo (whatever that

is) when this class is initialized, so there is always a cluster algo to be used even if its not added in the chain, is there a reason not to do this?  ie. add an else statement here (something like) if( !mClusterAlgoPtr ){ ... } else {

     LOG_INFO << "Using default fgt cluster algorithm" << endm;

     StFgtSimpleClusterAlgo* defaultAlgo = new StFgtSimpleClusterAlgo();

     setClusterAlgo(defaultAlgo);

}

-In documentation should also give some info on why there are 2 separate cluster algos?  Is one better than the other in general?  Which is the default?  Are there cases where the non-default is prefered?

-At the end of Make() need to remove all printf() and replace with probably LOG_DEBUG so don't get excessive strip/cluster info printed for every event in the standard chain...

 

Clustering Algos:

-StFgtSimpleClusterAlgo:

     -Probably needs more description on what steps actually are and what choices are made (both overview in header file and inline with code)

     -A few hard coded numbers eg. ordinate < 19 (line 106) and

numStrips<=10 (line 275) seem like fairly fundamental constants that might be better stored elsewhere?

 

-StFgtMaxClusterAlgo:

     -Probably needs more description on what steps actually are and what choices are made (both overview in header file and inline with code)

 

 

Finally some general thoughts/questions:

-Not sure where else to ask this, but does any of the logic in cluster/hit finding simplify if you were to create a StFgtStripCollection for both R and Phi?  It seems to me there are some places where you might benefit from being able to loop over only one plane at a time (this is what is done for the shower max planes in the EMCs), but its basically just a choice you have to make and then live with.

-Where do you plan to associate hits from R and Phi in the code?  I guess you have a StFgtPointMaker which is not included in this review to combine StFgtHits?  Will this be reviewed separately or maybe I shouldn't worry about it.

-Comment on bfc.C, currently all the library loading etc. are hard coded into bfc.C, but presumably there will be chain options created for all these pieces and an "fgtDat" or something that can be loaded in the common chain.  It would be good to do this and compare the output with the currently hard-coded bfc.C -Where are muDst containers, do you want them for fastOffline analysis and QA, etc?  Only a fraction of the data has event.root trees saved, right Jerome?