CAL Code Review
June 19, 2002
Richard gave his presentation
concerning the CalDigi package. Pointing out some items for the to do
list, including separating the execute routine into smaller methods and storing
constants in an input XML file.
Toby - should have XML file in each package with digi constants, where clients
would access the data via the IFile interface available in the xml package.
Leon - do the segmented logs give good results?
Sacha & Eric - not yet determined, needs more testing
Eric - xtal readout needs to include a mode that picks (forces) one of four diode modes
to be read out before event (i.e. not necessarily the best range).
Also, it is indeed bad to overwrite the readout values stored on the TDS when
adding noise.
Sasha made his presentation
introducing the CalRecon package.
Toby - What is the effect of shorter logs? Previously we were able to
assume a square geometry, that is no longer the case.
Sasha - does not dramatically change performance
Event Package
CalDigi Class
Heather - Add $Header$ and fix typo of ACD => ADC
Toby - whole description is in the brief section, we should separate the brief
from the detailed.
Sacha - It is necessary to insert a blank line to force the detailed
description.
Traudl - Doxygen should only take first line for brief
Heather - DocTF will check to see what Doxygen expects and update guidelines
accordingly
CalXtalRecData Class
Toby - there are both const and non-const versions of the getEnergy and
getPosition methods (that he inserted), only the const versions should be
retained.
It would be more straightforward to store the range as int rather than char.
Sasha - Eric wrote the class and tried to minimize the size of the data types
Heather - We have decided not to insert Doxy comments such as
"default constructor"
Karl - documenting parameters, there are places where "TDS INPUT" is
used and others where "INPUT" is used.
Heather - a fillStream method should be used rather than writeOut method.
CalCluster Class
Heather - It was noted in the comments that the CalClusterCol class will
be removed and replaced by an ObjectVector of CalCluster.
Karl - How/when to inherit from DataObject, ContainedObject ?
Toby - TDS classes generally use containedObject then use typedef's elsewhere
CalDigi Package
Heather - Mainpage should contain more information about the test routine
Richard - version does not appear on the mainpage for some reason
Berrie - need to measure and model noise in the instrument as function
of epoch and put that back in the simulations as e.g. pedestals and gains
Traudl - use MySql for calibration constants
Richard - CalDB only for instrument response function
Use of a DB for constants is a few months off.
Sasha - never thought to keep in DB all noise histograms,
rather than simply mean value and sigma for each channel.
Leon - Should use random triggers to simulate noise to simulate things
like pile up - "event blending" in generator
CalDigi
CalDigiAlg Class
Richard stepped through code, thought he inserted TDS INPUT and TDS OUTPUT in
all locations - but the version we were viewing did not contain them in all
places.
Heather - try to use different names for type and variable name, ex) SignalMap
and signalMap. Can an enumeration be used for the diodes?
Richard - should be possible to use an enumeration that is already defined.
CalRecon Package
Karl - Mainpages should link in the release.notes, rather than copying them in.
Sacha - had trouble getting the link to work, will look at the CalDigi package
for an example.
Heather - is the CalNumber parameter, which counts the number of
times CalClusterAlg is to be called, really necessary - that could be determined
at runtime?
Sacha - Leon inserted this parameter, but CalClusterAlg does not really use it.
Toby - Parameter is not necessary
Leon - It was originally put in to tell the 2 calls to CalClusterAlg apart, but
it sounds as though this parameter is not needed and may be removed.
Berrie - light(Energy) nonlinear (current approximation in simulation is linear), especially for protons which will be used for
calibrations - should use things like recent test results from CERN - need to estimate energy from multiple scattering
(in TKR in our case) as was done for EGRET
Leon - We do have method for estimating energy from multiple scattering (buried in pattern recognition routine currently)
Relying solely on hits may not be a good representation, a combination of methods
will be necessary at low energies
Mark - The current method is a combination since only hits within a certain
distance of the track is used. Energy correction is tricky re. specification of what actual
error is
CalXtalRecAlg Class
Heather - There should be global utilities for conversion, such as GeV=>MeV,
probably in the facilities package. When specifying TDS INPUT/OUTPUT it
would be better to provide full path or full EventModel variable name so that
users know to what is being referred to. Data members should have first letter of name lower case,
Leon - re. comment by Heather, that CLHEP classes should be used for vectors,... - he agrees but would be nontrivial
job to replace "mixed case" for Tkr, and won't be done without an edict (from data
structures task force ?)
Toby - agrees it would be nice to be consistent, but if it's not broke..
Heather - but we could suggest that all new code use the CLHEP classes
CalClusterAlg
Sasha - does not like including Minuit optimizer directly in code (Regis did this because for some reason
couldn't use it as a library call) - can we do this as a call to Root ?
Toby - line 706, should use const when retrieving data from TDS
line 721 construction of Vector should be done in a more OO way
Sasha - Cannot multiply a Vector in this fashion, trying to construct a weighted
position Vector.
Sasha - walk through of CalClustersAlg::execute ( )
Berrie - Can use direction from tracker to initialize fit
Sasha - We should if available, otherwise we use calorimeter direction
Berrie - no use of transverse information ?
Sasha - Actually only transverse info. used; don't know errors on
longitudinal, info. so though seems like we should, don't use that
CalDisplay Class
Toby - CalRep should be nested in CalDisplay (CalDisplay::CalRep)
Action Items for CAL group
Test to see if xtal segmentation, as planned, gives adequate positions.
Add "forced" range mode to CalXtalId and use it in the Event::CalDigi TDS
classes
Fix typo in Event::CalDigi class description: ACD => ADC
Add $Header$ to include history header from cvs
Make "brief" comment distinct from detailed comment by adding empty line
between brief and detailed comments. DocTF will check on this
In Event::CalXtalRecData - Get rid of non-const versions of getEnergy and
getPosition methods
Event::CalCluster: Use GAUDI "FillStream" instead of writeOut
Replace CalClusterCol with ObjectVector
CalDigi package mainpage: Add more info on test routine
CalDigi::CalDigiAlg - separate execute method into separate methods
CalRecon::CalDisplay - nest CalRep class in CalDisplay
For both CalDigi and CalRecon algorithms - use XML files to store constant parameters
Make sure TDS INPUT/OUPUT comments provide full path or full EventModel const name
CalRecon::CalClusterAlg - remove CalNumber jobOptions parameter
Action Items for Documentation Task Force
Check to see how Doxygen separates brief from detailed description and update guidelines accordingly.
H. Kelly, M. Stickman, K. Young Last Modified: 2004-08-04 15:40:00 -0700