TkrRecon Code Review II, Part 1
September 4, 2002
Presenters: Johann Cohen-Tanugi, Leon
Rochester, Tracy Usher
Reviewers: Xin Chen, Norm Graf, Heather Kelly, Hiro Tajima, Karl
Young.
The overview was split into 4 parts. Tracy described the overall architecture of TkrRecon, Leon spoke about clustering, and Johann described vertexing. Bill Atwood was going to speak about track finding and fitting but unfortunately was unable to attend; we will attempt to reschedule this part of the review, during Core Week (Sept. 9 -13) if possible. Tracy and Leon spoke on slides 1-9 and 10-14, respectively, of a single presentation (available in Powerpoint or PDF format). Johann had a separate web page. [Thank you, presenters, for providing such complete written presentations that there is no need for me to transcribe the oral version here. ed.]
Johann's page also pointed to a more technical document making use of LaTeX. He raised the question (and provided 3 possible answers) of where such documents ought to be kept.
Tracy noted that TkrRecon TDS classes need to be reviewed. Some useful quantities which should be stored there are not; conversely, some items now in the TDS are obsolete and should be removed.
(Heather) Missing description of job options parameters (prescribed by Doc TF standards).
(Richard) Should include some description of the TkrRecon test program and what it is meant to test.
(Leon) The test program uses a pre-existing data set of muons also used by Cal, so that simulation and digitization don't have to be part of the test program. We ought to have a common place to keep such files.
Toby wonders about the purpose of the last line in the requirements file:
make_fragment library_no_share
Leon could use some enlightenment also; the line is there because it's been there.
(various) use statements include no versioning information at all. Typically at least major version should be specified. Recently Traudl made a proposal of how we can handle versioning and related matters better; we hope to settle on a scheme during Core Week or soon thereafter.
We did a top-down walk-through of much of the code, starting with the driver Gaudi algorithm TkrReconAlg. [Note the generation of Doxygen documentation didn't work perfectly. In order to get to some source code files it may be necessary to bring up the file list.] The subalgorithms (also declared as Gaudi algorithms, one per processing stage) tend to be structured in a similar fashion, getting job options parameters in their initialize() method, getting input from the TDS and in turn creating new objects as output and registering them in the TDS. The actual algorithmic work is typically done one more level down (by classes declared to be Gaudi tools), allowing the choice of implementation to be deferred until run-time.
Comments specific to a particular class or file:
Richard wondered about the cut on clusters that are "too big". Leon says this is a relatively new addition which needs more thought; may want to change or eliminate it.
TkrCombPatRec has two modes of operation, depending on whether it has calorimeter information or not.
TkrCombPatRec.cxx is quite long, hence difficult to understand and maintain. Richard would like to see a text description of what gets done in the denser parts of the code. Karl wondered if the file could somehow be split up. Tracy has already been thinking along these lines and has some ideas. Leon thinks it may be possible to handle cases of x-y versus y-x arrangement of Si more generically, reducing the amount of code.
Leon was unable to get standard template library merge to work in the TkrRecon package; it did work in a little stand-alone program.
(Xin) Dead strips and hot strips are different, yet they are treated identically in the code. Will this change? (Leon) Dead and hot strips will be separately identifiable as such. Ultmately some code probably will handle them differently, but in many circumstances it actually is appropriate to treat them identically.
Need to branch in TkrTrackFitAlg::execute() to pick appropriate fitter tool, depending on which tool was used for track find step.
Several other comments, while made in the context of a particular class or file, may be relevant to the package as a whole:
Check status codes (example of ignored status in TkrClusterAlg)
Public access routines should be const
Cuts, such as those used to define good clusters, should be job options parameters or in some other way (xml file?) be made accessible to and changeable by users. At the very least, these and other constants should not be buried as literals in code. If they all have names and are defined in a single place it is a simple matter to change the way they get their values later.
Leon has been engaged in ongoing work to get hard-coded parameters out of the code and to get them from other sources (e.g. geometry description) when appropriate.
(Heather) Why is the an iterator initialized in line 47 of TkrMakeClusters.cxx but not used to control the immediately following loop? (Leon) No particular reason. Should iterators typically be used for loop control? Are incrementing indices appropriate in case we're iterating over a vector? Or should this be done on a case-by-case basis? (Toby) Iterators are preferred because safer.
(Heather) dynamic_cast is not necessary on line 37 of TkrFitTool.cxx and in similar situations.
Need to eliminate or at least lessen the number of warning generate by Linux compile.
Leon prefers explicit "get" at the front access routine names to lessen ambiguity. This is in fact what Doc TF recommends.
Action items for TkrRecon group
Add comprehensive list of job options parameters to mainpage.h. Add description of test program.
Reconsider "too big" cut on clusters: should there be one? If so, is this the right number? Is it the right style of cut?
Provide separate text description of TkrComboPatRec (as has been done for some of the other TkrRecon code) since it is complex. One way or another, reduce size of the implementation file.
Make access routines const. Give them names starting with "get"
Check returned status codes.
Get rid of literal constants buried in code.
Consider implementing a scheme to get user-settable parameters from an XML file.
Use iterators where appropriate.
Don't use redundant dynamic_cast
Reduce number of Linux compile errors.
Action items for Documentation Task Force
J. Bogart Last Modified: 2004-08-04 15:41:00 -0700