Hi Steve et al, On Sat, 26 Jan 2002 10:27:34 -0600 "Stephen J. Sanders" <ssanders@ku.edu> wrote concerning "Si/Tile/Mult rdo module changes; BrMrsTrackingModule.h fix": > Hi, > First, I added a definition of hTrigger to BrMrsTrackingMdoule.h. It should "fTrigger" or "fTriggerHisto" according to out coding conventions. See 'The Guide'. > Somewhat more significant, although I believe transparent, changes > were made to the multipiclity modules. Before the threshold cut was > done very early, preventing the pedestal from being including in > the various histograms. No, the point was not to not have it in the histograms (double negation :-); rather the point was to flag below-thershold channels as early as possible, so save computing time and simplify the code. Please, when you need to pass argumetns by reference to a function - as opposed to by value - do not use pointer, but use references. References is what you want, and pointers and references are not the same thing! Remember, this is C++ not C/Fortran77 :-) - we can do better. So instead of CalibrateAdc(Int_t adc, Bool_t * BelowThreshold) do CalibrateAdc(Int_t adc, Bool_t& belowThreshold) Also, in our coding conventions (see 'The Guide') only method names start with capital letters. Also, do not use the plain C++ true and false constants, but use the ROOT kTRUE and kFALSE. I've made these changes. > I have now delayed applying the threshold cut so that I can use the > histograms, with pedestals, for a new mult calibration module. (I > hope to add the new module to brat very shortly...the development > version is in sjs_app/modules) Speaking of calibrations, I asked you for some just before new years - how are they coming along? Glad to see that you're now developing a proper module for the TMA and SMA calibrations. Now some thought about your module in sjs_app/modules/[inc|src]/multCalibModule.[h,cxx] As far as I can tell, this module does pedestals and "single MIP peaks" for both the TMA and SMA, using the histograms of BrMultRdoModule. This is bad for several reasons: 1) It creates a chicken-egg paradox: - To use BrMultRdoModule you must have the calibrations. - You get the calibrtions from multCalibModule. - However, multCalibModule assumes you have used BrMultRdoModule! If you need to get the energy, the best thing to do is to subclass your calibration class from BrMultRdoModule. Hence, you have all the nessecary methods avaliable, and those you do not need, you can overload to do nothing. So C++ can solve the Chicken-egg paradox :-) 2) One module must do only one thing. You should split the module multCalibModule into two modules: - BrMultPedCalModule which does only pedestals. - BrMultMipCalModule which does only "single MIP peaks" This is to have the flexibility of running various kinds of calibration passes. For example, one can, as a calibration shift leader, set up a job that does pedestals (and only pedestals) for all detectors that need that kind of calibration. Another job can be setup to do "Single MIP peak" calibrations for those detectors that need that. I sent you a mail some time ago about this, outlining how I thougt you could go about it. Please have a look at the exisiting code in brat/modules/calib/mult/Br[Tile|Si]PedCalModule.h and see if you cannot use that. 3) A module must only do one thing for one detector. This means that you should do calibrations for _either_ the TMA _or_ the SMA in the modules. Given that the code is identical, you can abstract to one class for one kind of calibration e.g., BrMultPedCalModule. Again, this gives us the kind of flexibility that we want when setting up calibration passes. Finally, it doesn't make sense to update the calibration constants outside of the End method. Calibrations _must_ last _at_least_ a run, and so the appropiate time for the update is at the end of a run-level, which is in End. Therefor, the statement if(fEvntCnt%2000==0) FindPedestals(); in the Event meothod is bad! What about the rest of the calibrations needed for the TMA and SMA, like * TMA ADC gap (see if brat/modules/calib/mult/BrTileAdcGapCalModule.h is something you can use). * SMA FERA non-linearity correction. * TMA and SMA calibrated ADC to energy conversion functions. * TMA and SMA energy to multiplicity conversion functions. Now for some more general comments on your code: * Write comments! This _very_ important. No one will understand your code if you do not write comments. If I had a penny for each time I said that ... ah well, you know the drill :-) * Give the histograms a readable title - that helps people get an overview of what is in the directory. For example, instead of fbbVsTrack = new TH2F("fbbVsTrack","fbbVsTrack", 321,-80,80,321,-80,80); do fBbVsTrack = new TH2F("bbVsTrack","Beam-Beam vs. TPM1 Track v_{z}", 321,-80,80,321,-80,80); and please put titles on the axis fBbVsTrack->SetXTitle("BB v_{z}"); fBbVsTrack->SetYTitle("TPM1 track v_{z}"); * Do not use sprintf and character arrays. Use TString and Form. That is, instead of Char_t histname[30]; for(Int_t i = 0; i<fVtxCuts; i++) { sprintf(histname,"siCalib_%d",i); fSiVtxCalib[i]=new TH2F(histname,histname, nSiSingle,0.5,nSiSingle+0.5,21 do for(Int_t i = 0; i<fVtxCuts; i++) { sprintf(histname, fSiVtxCalib[i]=new TH2F(Form("siCalib_%03d",i), Form("SMA Raw ADC %d", i), nSiSingle,0.5,nSiSingle+0.5,21 The "%03d" in the format string, makes the numbers come out as 001, 002, ..., 212, 213, ... rather than 1, 2, ..., 212, 213, ... The later has the unfortunate effect that sorting the histogram names, puts histograms in the order 1, 10, 11, ..., 2, 20, 21, ..., 212, 213, ..., 3, ... which is not what you want. * Stop, Error, Abort, Info, and Debug, all take variadic arguments; that is they have a parameter list like for example Abort(const Char_t* where, const Char_t*, ...) so do not do sprintf(errormsg,"%s not found",hname); Abort("FindSingMipPeak",errormsg); but do directly Abort("FindSingMipPeak", "%s not found", hname); The above is a C anacrosim you want to avoid. * Instead of doing tests like if( th2 == 0 ) Abort("FindPedestals","Si/singleAdc histogram not found"); else { // Do what ever make an exit point if the condition is true, like if(!th2) { Abort("FindPedestals","Si/singleAdc histogram not found"); return; } // Do what ever or event try { if (!th2) throw new BrFatal("FindPedestals", "Si/singleAdc histogram not found"); // Do what ever } catch (BrException* e) { cerr << *e << endl; // Do clean-up here e->Exec(); } * Use the form [head][Tail]+ for local variables. That is, instead of Int_t MaxBin = th1->GetMaximumBin(); Char_t hname[20]; write Int_t maxBin = th1->GetMaximumBin(); Char_t histoName[20]; Well, at least be consistent, and try to give even local vaiables meaningful names. This is not Fortran4 - we can have more than 6 characters in identifers :-) * Try not to go beyond column 70, since it makes it hard to read the code in a terminal. Ok this is one point where you can say Fortran forced you to behave nicely :-) Ok, finally the changelog of my resent changes: 2002-01-27 Christian Holm Christensen <cholm@rcas0014.rcf.bnl.gov> * modules/rdo/BrTileRdoModule.cxx, modules/rdo/BrSiRdoModule.cxx, modules/rdo/BrMultRdoModule.h, modules/rdo/BrMultRdoModule.cxx: Use pass-by-reference ('&') rather than pointers ('*') in the method BrMultRdoModule::CalibrateAdc, since that's the proper C++ way to go about these things. 2002-01-26 Flemming Videbaek, BNL, x4106 <videbaek@rcas0014.rcf.bnl.gov> * modules/track/dc/BrDC2DTrackFinder.cxx: Found a nasty memory leak in the DeleteTrack. The bad trackcandidate was removed from the list that was an TObjArray. This does not call the destructor of the object. This in contract to the behaviour of TClonesArray where the destructor is called. Nore the original code was written using TClonesArray. A elaboration to Flemmings changes. Memory is _never_ free'd by TClonesArray - that's the whole point of TClonesArray, to reuse the allocated memory. What happens in TClonesArray::Remove(TObject*), is that the destructer is called, _but_the_associated_memory_is_not_free'd_. In TObjArray::Remove(TObject*) memory isn't free'd either, and destructors are not called either, regardless of the fIsOwner flag. Remember, a TCollection and it's subclasses, except TClonesArray, basically only holds references to the objects put in the collection. It only does allocation and deallocation if you ask it to, via SetOwner(kTRUE). So the objects are not "in" the TCollection, rather they a "used by" the TCollection. That means that adding and removing entries does not change the entries in anyway. Clearing and Deleting from a collection _can_ change the entries if fIsOwner is true. Yours, Christian Holm Christensen ------------------------------------------- Address: Sankt Hansgade 23, 1. th. Phone: (+45) 35 35 96 91 DK-2200 Copenhagen N Cell: (+45) 28 82 16 23 Denmark Office: (+45) 353 25 305 Email: cholm@nbi.dk Web: www.nbi.dk/~cholm
This archive was generated by hypermail 2b30 : Sun Jan 27 2002 - 11:32:37 EST