Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permanently enabling method 3 #11507

Merged
merged 2 commits into from
Oct 1, 2015
Merged

Permanently enabling method 3 #11507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ec547d
Permanently enabling method 3
Sep 26, 2015
bb4492a
reverting code optimizations
Sep 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions RecoLocalCalo/HcalRecAlgos/interface/HcalSimpleRecAlgo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ class HcalSimpleRecAlgo {
HcalSimpleRecAlgo(bool correctForTimeslew,
bool correctForContainment, float fixedPhaseNs);

/** Simple constructor for PMT-based detectors */
HcalSimpleRecAlgo();

void beginRun(edm::EventSetup const & es);
void endRun();

Expand Down Expand Up @@ -80,17 +77,17 @@ class HcalSimpleRecAlgo {
HcalCalibRecHit reconstruct(const HcalCalibDataFrame& digi, int first, int toadd, const HcalCoder& coder, const HcalCalibrations& calibs) const;

void setpuCorrMethod(int method){
puCorrMethod_ = method; if( puCorrMethod_ == 2 ) psFitOOTpuCorr_ = std::auto_ptr<PulseShapeFitOOTPileupCorrection>(new PulseShapeFitOOTPileupCorrection());
if( puCorrMethod_ == 3) hltOOTpuCorr_ = std::auto_ptr<HcalDeterministicFit>(new HcalDeterministicFit());
puCorrMethod_ = method;
if( puCorrMethod_ == 2 )
psFitOOTpuCorr_ = std::auto_ptr<PulseShapeFitOOTPileupCorrection>(new PulseShapeFitOOTPileupCorrection());
}

void setpuCorrParams(bool iPedestalConstraint, bool iTimeConstraint,bool iAddPulseJitter,bool iUnConstrainedFit,bool iApplyTimeSlew,
double iTS4Min, double iTS4Max, double iPulseJitter,double iTimeMean,double iTimeSig,double iPedMean,double iPedSig,
double iNoise,double iTMin,double iTMax,
double its3Chi2,double its4Chi2,double its345Chi2,double iChargeThreshold, int iFitTimes);
void setMeth3Params(int iPedSubMethod, float iPedSubThreshold, int iTimeSlewParsType, std::vector<double> iTimeSlewPars, double irespCorrM3);

std::auto_ptr<PedestalSub> pedSubFxn_= std::auto_ptr<PedestalSub>(new PedestalSub());

private:
bool correctForTimeslew_;
bool correctForPulse_;
Expand All @@ -111,8 +108,13 @@ class HcalSimpleRecAlgo {

std::auto_ptr<PulseShapeFitOOTPileupCorrection> psFitOOTpuCorr_;

std::auto_ptr<PedestalSub> pedSubFxn_;

// S.Brandt Feb19 : Add a pointer to the HLT algo
std::auto_ptr<HcalDeterministicFit> hltOOTpuCorr_;

// Speed up the code a little bit by not allocating a vector every time
mutable std::vector<double> vectorBuffer_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the size of the saving from this buffer?
I'd much rather see no mutables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this.
I don't see much benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The saving does not come from the "size" of the vector. It is instead about not allocating (and then deallocating) the memory from the heap for each calorimeter cell processed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are several more allocations in the methods called internally.
So, if a fix needs to be done, better do it properly.

Your change with a mutable opens a possibility to pass information between hits.
This is not good.

Cleaning and clearing is apparently done correctly now, but this makes the code much more fragile in the future developments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is with 25ns hlt in 760pre5
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_7_6_0_pre5-orig.256729.step2-hlt.pp/797

i don't see any cpu cost reasons for this buffer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocations internal to "Method 2" and "Method 3" classes are, of course, irrelevant to this discussion. Bug the authors of the corresponding classes if you are so inclined.

The change with a mutable, of course, does not open any possibility to pass information between hits. The vector is cleared any time it is used.

Any code is "fragile" to some degree. I believe, the advantages outweigh the drawbacks in this particular case. The "mutable" keyword is included into the language precisely for the uses similar to this.

The cost reasons are obvious: we avoid allocating the contents of the vector on the heap for every hcal rechit.

};

#endif
115 changes: 60 additions & 55 deletions RecoLocalCalo/HcalRecAlgos/src/HcalSimpleRecAlgo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,10 @@ HcalSimpleRecAlgo::HcalSimpleRecAlgo(bool correctForTimeslew, bool correctForPul
correctForTimeslew_(correctForTimeslew),
correctForPulse_(correctForPulse),
phaseNS_(phaseNS), runnum_(0), setLeakCorrection_(false), puCorrMethod_(0)
{

pulseCorr_ = std::auto_ptr<HcalPulseContainmentManager>(
new HcalPulseContainmentManager(MaximumFractionalError)
);
}


HcalSimpleRecAlgo::HcalSimpleRecAlgo() :
correctForTimeslew_(false), runnum_(0), puCorrMethod_(0)
{
{
pulseCorr_ = std::auto_ptr<HcalPulseContainmentManager>(new HcalPulseContainmentManager(MaximumFractionalError));
pedSubFxn_ = std::auto_ptr<PedestalSub>(new PedestalSub());
hltOOTpuCorr_ = std::auto_ptr<HcalDeterministicFit>(new HcalDeterministicFit());
}


Expand Down Expand Up @@ -324,9 +317,9 @@ namespace HcalSimpleRecAlgoImpl {
const HcalTimeSlew::BiasSetting slewFlavor,
const int runnum, const bool useLeak,
const AbsOOTPileupCorrection* pileupCorrection,
const BunchXParameter* bxInfo, const unsigned lenInfo, const int puCorrMethod, const PulseShapeFitOOTPileupCorrection * psFitOOTpuCorr, HcalDeterministicFit * hltOOTpuCorr, PedestalSub * hltPedSub /* whatever don't know what to do with the pointer...*/)// const on end
const BunchXParameter* bxInfo, const unsigned lenInfo, const int puCorrMethod, const PulseShapeFitOOTPileupCorrection * psFitOOTpuCorr, HcalDeterministicFit * hltOOTpuCorr, PedestalSub * hltPedSub /* whatever don't know what to do with the pointer...*/, std::vector<double>& buffer)// const on end
{
double fc_ampl =0, ampl =0, uncorr_ampl =0, maxA = -1.e300;
double fc_ampl =0, ampl =0, uncorr_ampl =0, m3_ampl =0, maxA = -1.e300;
int nRead = 0, maxI = -1;
bool leakCorrApplied = false;
float t0 =0, t2 =0;
Expand Down Expand Up @@ -361,36 +354,39 @@ namespace HcalSimpleRecAlgoImpl {

// Note that uncorr_ampl is always set from outside of method 2!
if( puCorrMethod == 2 ){
std::vector<double> correctedOutput;
buffer.clear();

CaloSamples cs;
coder.adc2fC(digi,cs);
std::vector<int> capidvec;
CaloSamples cs;
coder.adc2fC(digi,cs);
std::vector<int> capidvec;
for(int ip=0; ip<cs.size(); ip++){
const int capid = digi[ip].capid();
capidvec.push_back(capid);
}
psFitOOTpuCorr->apply(cs, capidvec, calibs, correctedOutput);
if( correctedOutput.back() == 0 && correctedOutput.size() >1 ){
time = correctedOutput[1]; ampl = correctedOutput[0];
psFitOOTpuCorr->apply(cs, capidvec, calibs, buffer);
if( buffer.back() == 0 && buffer.size() >1 ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the size is used mainly to signal the quality of the fit.
If preallocation appears to give a significant gain, just start with a fixed size vector or even an std::array and pass the fit status by value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I don't really want to change the interface of two classes written by somebody else.

time = buffer[1]; ampl = buffer[0];
}
}

// S. Brandt - Feb 19th : Adding Section for HLT
// Turn on HLT here with puCorrMethod = 3
if ( puCorrMethod == 3){
std::vector<double> hltCorrOutput;
// Run "Method 3" all the time.
{
buffer.clear();

CaloSamples cs;
coder.adc2fC(digi,cs);
std::vector<int> capidvec;
for(int ip=0; ip<cs.size(); ip++){
const int capid = digi[ip].capid();
capidvec.push_back(capid);
}
hltOOTpuCorr->apply(cs, capidvec, calibs, digi, hltCorrOutput);
if( hltCorrOutput.size() > 1 ){
time = hltCorrOutput[1]; ampl = hltCorrOutput[0];
hltOOTpuCorr->apply(cs, capidvec, calibs, digi, buffer);
if( buffer.size() > 1 ){
m3_ampl = buffer[0];
if (puCorrMethod == 3) {
time = buffer[1]; ampl = buffer[0];
}
}
}

Expand All @@ -400,33 +396,36 @@ namespace HcalSimpleRecAlgoImpl {
if (cell.subdet() == HcalBarrel) {
const int ieta = cell.ieta();
const int iphi = cell.iphi();
ampl *= eCorr(ieta, iphi, ampl, runnum);
uncorr_ampl *= eCorr(ieta, iphi, uncorr_ampl, runnum);
ampl *= eCorr(ieta, iphi, ampl, runnum);
m3_ampl *= eCorr(ieta, iphi, m3_ampl, runnum);
}
}

// Correction for a leak to pre-sample
if(useLeak && !leakCorrApplied) {
ampl *= leakCorr(ampl);
uncorr_ampl *= leakCorr(uncorr_ampl);
uncorr_ampl *= leakCorr(uncorr_ampl);
if (puCorrMethod < 2)
ampl *= leakCorr(ampl);
}

RecHit rh(digi.id(),ampl,time);
setRawEnergy(rh, static_cast<float>(uncorr_ampl));
setAuxEnergy(rh, static_cast<float>(m3_ampl));
return rh;
}

template<class Digi, class RecHit>
template<class Digi, class RecHit>
inline RecHit recoHBHE(const Digi& digi, const HcalCoder& coder,
const HcalCalibrations& calibs,
const int ifirst, const int n, const bool slewCorrect,
const bool pulseCorrect, const HcalPulseContainmentCorrection* corr,
const HcalTimeSlew::BiasSetting slewFlavor,
const int runnum, const bool useLeak,
const AbsOOTPileupCorrection* pileupCorrection,
const BunchXParameter* bxInfo, const unsigned lenInfo, const int puCorrMethod, const PulseShapeFitOOTPileupCorrection * psFitOOTpuCorr, HcalDeterministicFit * hltOOTpuCorr, PedestalSub * hltPedSub )// const on end
const BunchXParameter* bxInfo, const unsigned lenInfo, const int puCorrMethod, const PulseShapeFitOOTPileupCorrection * psFitOOTpuCorr, HcalDeterministicFit * hltOOTpuCorr, PedestalSub * hltPedSub, std::vector<double>& buffer)// const on end
{
double fc_ampl =0, ampl =0, uncorr_ampl =0, maxA = -1.e300;
double fc_ampl =0, ampl =0, uncorr_ampl =0, m3_ampl =0, maxA = -1.e300;
int nRead = 0, maxI = -1;
bool leakCorrApplied = false;
float t0 =0, t2 =0;
Expand Down Expand Up @@ -462,63 +461,69 @@ template<class Digi, class RecHit>

// Note that uncorr_ampl is always set from outside of method 2!
if( puCorrMethod == 2 ){
std::vector<double> correctedOutput;
buffer.clear();

CaloSamples cs;
coder.adc2fC(digi,cs);
std::vector<int> capidvec;
for(int ip=0; ip<cs.size(); ip++){
const int capid = digi[ip].capid();
capidvec.push_back(capid);
}
psFitOOTpuCorr->apply(cs, capidvec, calibs, correctedOutput);
if( correctedOutput.back() == 0 && correctedOutput.size() >1 ){
time = correctedOutput[1]; ampl = correctedOutput[0];
useTriple = correctedOutput[4];
psFitOOTpuCorr->apply(cs, capidvec, calibs, buffer);
if( buffer.back() == 0 && buffer.size() >1 ){
time = buffer[1]; ampl = buffer[0];
useTriple = buffer[4];
}
}

// S. Brandt - Feb 19th : Adding Section for HLT
// Turn on HLT here with puCorrMethod = 3
if ( puCorrMethod == 3){
std::vector<double> hltCorrOutput;
// Run "Method 3" all the time.
{
buffer.clear();

CaloSamples cs;
coder.adc2fC(digi,cs);
std::vector<int> capidvec;
for(int ip=0; ip<cs.size(); ip++){
const int capid = digi[ip].capid();
capidvec.push_back(capid);
}
hltOOTpuCorr->apply(cs, capidvec, calibs, digi, hltCorrOutput);
if( hltCorrOutput.size() > 1 ){
time = hltCorrOutput[1]; ampl = hltCorrOutput[0];
hltOOTpuCorr->apply(cs, capidvec, calibs, digi, buffer);
if (buffer.size() > 1) {
m3_ampl = buffer[0];
if (puCorrMethod == 3) {
time = buffer[1]; ampl = buffer[0];
}
}
}

// Temporary hack to apply energy-dependent corrections to some HB- cells
if (runnum > 0) {
const HcalDetId& cell = digi.id();
if (cell.subdet() == HcalBarrel) {
const int ieta = cell.ieta();
const int iphi = cell.iphi();
ampl *= eCorr(ieta, iphi, ampl, runnum);
uncorr_ampl *= eCorr(ieta, iphi, uncorr_ampl, runnum);
ampl *= eCorr(ieta, iphi, ampl, runnum);
m3_ampl *= eCorr(ieta, iphi, m3_ampl, runnum);
}
}

// Correction for a leak to pre-sample
if(useLeak && !leakCorrApplied) {
ampl *= leakCorr(ampl);
uncorr_ampl *= leakCorr(uncorr_ampl);
if (puCorrMethod < 2)
ampl *= leakCorr(ampl);
}

HBHERecHit rh(digi.id(),ampl,time);
if(useTriple)
{
rh.setFlagField(1, HcalCaloFlagLabels::HBHEPulseFitBit);
}
setRawEnergy(rh, static_cast<float>(uncorr_ampl));
setAuxEnergy(rh, static_cast<float>(m3_ampl));
return rh;
}
}
Expand All @@ -531,7 +536,7 @@ HBHERecHit HcalSimpleRecAlgo::reconstruct(const HBHEDataFrame& digi, int first,
HcalTimeSlew::Medium,
runnum_, setLeakCorrection_,
hbhePileupCorr_.get(),
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get());
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get(),vectorBuffer_);
}


Expand All @@ -541,7 +546,7 @@ HORecHit HcalSimpleRecAlgo::reconstruct(const HODataFrame& digi, int first, int
pulseCorr_->get(digi.id(), toadd, phaseNS_),
HcalTimeSlew::Slow,
runnum_, false, hoPileupCorr_.get(),
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get());
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get(),vectorBuffer_);
}


Expand All @@ -551,7 +556,7 @@ HcalCalibRecHit HcalSimpleRecAlgo::reconstruct(const HcalCalibDataFrame& digi, i
pulseCorr_->get(digi.id(), toadd, phaseNS_),
HcalTimeSlew::Fast,
runnum_, false, 0,
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get());
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get(),vectorBuffer_);
}


Expand All @@ -561,7 +566,7 @@ HBHERecHit HcalSimpleRecAlgo::reconstructHBHEUpgrade(const HcalUpgradeDataFrame&
pulseCorr_->get(digi.id(), toadd, phaseNS_),
HcalTimeSlew::Medium, 0, false,
hbhePileupCorr_.get(),
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get());
bunchCrossingInfo_, lenBunchCrossingInfo_, puCorrMethod_, psFitOOTpuCorr_.get(),/*hlt*/hltOOTpuCorr_.get(),pedSubFxn_.get(),vectorBuffer_);
HcalTDCReco tdcReco;
tdcReco.reconstruct(digi, result);
return result;
Expand Down
19 changes: 8 additions & 11 deletions RecoLocalCalo/HcalRecProducers/src/HcalHitReconstructor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ HcalHitReconstructor::HcalHitReconstructor(edm::ParameterSet const& conf):
mcOOTCorrectionCategory_("MC"),
setPileupCorrection_(0),
paramTS(0),
puCorrMethod_(conf.existsAs<int>("puCorrMethod") ? conf.getParameter<int>("puCorrMethod") : 0),
puCorrMethod_(conf.getParameter<int>("puCorrMethod")),
cntprtCorrMethod_(0),
first_(true)

Expand Down Expand Up @@ -270,16 +270,13 @@ HcalHitReconstructor::HcalHitReconstructor(edm::ParameterSet const& conf):
conf.getParameter<int> ("fitTimes")
);
}
if(puCorrMethod_ == 3) {
reco_.setMeth3Params(
conf.getParameter<int> ("pedestalSubtractionType"),
conf.getParameter<double> ("pedestalUpperLimit"),
conf.getParameter<int> ("timeSlewParsType"),
conf.getParameter<std::vector<double> >("timeSlewPars"),
conf.getParameter<double> ("respCorrM3")
);
}

reco_.setMeth3Params(
conf.getParameter<int> ("pedestalSubtractionType"),
conf.getParameter<double> ("pedestalUpperLimit"),
conf.getParameter<int> ("timeSlewParsType"),
conf.getParameter<std::vector<double> >("timeSlewPars"),
conf.getParameter<double> ("respCorrM3")
);
}

void HcalHitReconstructor::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
Expand Down