Hi Christian, Thanks for the advice. The module is now working correctly so once I work on the changes that you suggest I'll add it to brat...this should greatly simplify calibrating additional runs, as you and others have requested. ...steve Christian Holm Christensen wrote: >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:50:57 EST