Minutes of the DSTF Review of the TkrRecon TDS Classes

Leon Rochester, 02-Feb-2002

Present (in order of arrival)

Steve, Richard, Heather, Traudl, Tracy, Bill, Norman Graf, Seth, Toby, Michael, Hiro, Joanne, Sasha

Norman was here as an outside reviewer. Thanks, Norman!

Tracy's presentation

Tracy gave a very good overview of how TkrRecon works. (I learned a lot!)

• There were several questions about the strategy of putting all the TDS classes in GlastEvent. It would seem that this is actually mandated by technical considerations (although I'm not 100% sure even now). It looks like it will be possible to achieve this, that is to remove all dependence on TkrRecon from these classes. We think we've done it already, but won't know for sure until we try.
• Toby noticed an enum in global space in TkrClusters.
• Norman commented that there seems to be some duplication of data members between the TkrPlaneHit and TkrCluster objects. Also a question about whether all of the hit types (MEAS, PRED, FIT, SMOOTH) need to be stored for every hit.
• Norman asked where the information where the information about the reconstruction history of the track was: which algorithms, maybe which versions?
• Toby pointed out that any classes going into GlastEvent should be in a namespace (tkr).
• Traudl suggested that the private member m_flag doesn't belong in TkrCluster because it gets set after the data are put onto the TDS. There were questions later whether immutability is a requirement for TDS data. The variable in question is not an essential part of the invariant of the class; that is, its value doesn't determine if the class is in a valid state.

Heather's review

Heather's review concentrated on formal aspects of the classes: naming, consistency, use of namespaces, documentation, formatting.  There were many useful suggestions.

• We decided that TkrClusters should be renamed TkrClusterCol, even though it isn't a standard Gaudi Container.
• Toby reiterated that a namespace is needed for classes going into GlastEvent, and conversely, classes that live inside TkrRecon don't need the "Tkr" prefix on their names.
• In response to Heather's suggestion that enums be explicitly initialized ( enum {eeny=0, meeny=1, miney=2, moe=3}), Norman responded that any code which needs to know the value of the enum is probably not correctly written. I think this may be true for code using the "view" variables. We should discuss this further with him to see if there is a better way here.
• Layer, plane, bilayer, ilayer, kplane, etc., is a bit of a mess. Can we straighten it out? Also what do we do about renumbering the layers?  Technically Toby is right that the numbering of the bilayers doesn't necessarily have to correspond to that of the trays (physical thing), but it would be cleaner if it did. It would also be nice if we could do the reconstruction without resorting to actual layer numbers. Maybe a start would be a idents::bilayer, with methods like layerCountFromTop() and layerCountFromBottom().
• Norman suggested the use of an output stream to replace the writeOut() method of some of the classes. We used to do this. Toby suggested using the same name as the DataObject virtual method that handles that function.

Review of class content

We started to go through the content of the classes, to make sure that nothing was missing, and to check for consistency. We started with TkrCluster:

• m_id:  This identifies the sequential order of the clusters, but is not actually used in the methods that access the clusters. Those methods rely on the clusters being in the order of their creation. It is however used to store an index to the cluster. But we never check that the id matches the position in the list.  Should the code be rewritten to use pointers to the clusters instead of id's?
• chip(): this is just (strip number)/64. A chip handles 64 strips, so in general there's no space between the strips from adjacent strips (unlike the case for ladders). The problem is that the cluster may cover more than one chip. It would seem that chip() is a better concept for the digis, and should probably be moved there, or could be a method of a yet non-existent idents::strip.
• strip() and size():  why are these double?  strip() because it's the centroid of the cluster, which can be half-integer; size because it's mostly used in places where the result is a double.  For strip() maybe a better name would be getCentroid(); for getSize(), an int would probably be more logical since it is really a number of strips.

Then we ran out of time.  We need to finish this part sometime soon; it looked like it would be useful!