HCAL 25ns reco update #9154
HCAL 25ns reco update #9154
Conversation
…or Deterministic Fit configurable
…ith a single pulse or three pulses
|
A new Pull Request was created by @kfiekas for CMSSW_7_5_X. HCAL 25ns reco update It involves the following packages: CalibCalorimetry/HcalAlgos @nclopezo, @cvuosalo, @cerminar, @cmsbuild, @diguida, @slava77, @mmusich can you please review it and eventually sign? Thanks. |
|
@cmsbuild please test |
| enum BiasSetting { Slow=0, Medium=1, Fast=2 }; | ||
|
|
||
| /** \brief Returns the amount (ns) by which a pulse of the given | ||
| number of fC will be delayed by the timeslew effect, for the | ||
| specified bias setting. */ | ||
| static double delay(double fC, BiasSetting bias=Medium); | ||
| static double delay(double fC, ParaSource source=TestStand, BiasSetting bias=Medium); | ||
| static double delay(double fC, ParaSource source=TestStand, BiasSetting bias=Medium, double par0=9.27638, double par1=-2.05585); |
There was a problem hiding this comment.
@kfiekas Sorry, this interface declaration is not acceptable.
Putting magic numbers as default in a static member function is bad code implementation.
You can declare static constexpr double (float is not enough?) precision data members, and use them as default (so you also document the parameters)
|
The tests are being triggered in jenkins. |
|
|
||
| if (source==TestStand) { | ||
| return HcalTimeSlew::delay(fC, bias); | ||
| } | ||
| else if (source==Data) { | ||
| //from john 2/20 talk: indico.cern.ch/event/375365/contribution/9/material/slides/5.pdf | ||
| return 13.98-3.20*log(fC+32)-2.82965+10; | ||
| return std::min(6.0,10.2627-2.41281*log(fC)); |
There was a problem hiding this comment.
@kfiekas again, please avoid magic numbers (even if documented on Indico!)
| if(TMath::Abs(cell.ieta())<16){ | ||
| fpar0 = fparhb0; | ||
| fpar1 = fparhb1; | ||
| }else if(TMath::Abs(cell.ieta())==16||TMath::Abs(cell.ieta())==17){ |
There was a problem hiding this comment.
@kfiekas what do 16 and 17 mean?
There was a problem hiding this comment.
@diguida 16 and 17 should be the ieta values for transition region between HB and HE.
There was a problem hiding this comment.
here and the rest: use std::abs
There was a problem hiding this comment.
@xmniu thanks for the clarifications.
So, what about an array with the ieta of transition region?
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @smuzaffar |
|
May we have it merged? Thank you. |
|
would be better if we had the PR for the GT with PF cluster calibration at the same time
|
|
Ah... indeed. On Wed, 27 May 2015, David Lange wrote:
|
|
@davidlange6 @abdoulline the GTs with the new version of |
|
+1 |
Lowered the Method 2 thresholds, no use of Method0 for low-energy pulses anymore and 100fC switchover from the three-pulse fit to the one-pulse fit
Included a negative energy cut for Method2
Included a RecHit flag depending on whether Method2 fit was performed with a single pulse or three pulses
Made time slew parameters for Method3 configurable