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

Spike cleaner fix #17859

Merged
merged 7 commits into from
Aug 4, 2017
Merged

Spike cleaner fix #17859

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed7681a
Spikecleaning fix
knash Mar 8, 2017
883acd9
Merge remote-tracking branch 'official-cmssw/master' into HFchanges
knash Apr 4, 2017
00676b2
mergetest
knash Apr 4, 2017
d1ea66f
Address PR comments
knash Apr 7, 2017
ef82214
Rolling back PR comment change as it might affect the masked hit index
knash Apr 19, 2017
1d42f02
Turning off HF spike cleaning
knash Jun 7, 2017
88c30ee
implement latest comments. No change in functionality
knash Jul 14, 2017
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
47 changes: 46 additions & 1 deletion RecoParticleFlow/PFClusterProducer/src/SpikeAndDoubleSpikeCleaner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,55 @@ clean(const edm::Handle<reco::PFRecHitCollection>& input,
}
const spike_cleaning& clean = _thresholds.find(hitlayer)->second;
if( rechit.energy() < clean._singleSpikeThresh ) continue;



//Fix needed for HF
float compsumE = 0.0;
if ((hitlayer==12 or hitlayer==11))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these magic numbers be get from some configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This says to only apply the fix only to the HF layers. I think this should be hard coded, as any other values would be buggy

Copy link
Contributor

Choose a reason for hiding this comment

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

11 -> PFLayer::HF_EM
12 -> PFLayer::HF_HAD

{
int predphi =2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, I would put the declaration below together with the condition under which predphi is set to 4.

All in all, I cannot understand what this code is doing, would be nice to add a few comments, in particular a general comment explaining the problem you want to solve and how you can solve it ("Fix needed for HF" is too generic). When I understand better, I'll be able to comment further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, in the next version there will be more verbose comments.

This section checks if a rechit is from the HF region and if so it adds additional energy from the companion rechits (ie HF_Had <-> HF_EM) to the total surrounding energy


int comp = std::abs(hitlayer-13);
Copy link
Contributor

Choose a reason for hiding this comment

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

"13" here is quite likely "12+1", where "12" can be extracted from some enum or configured list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this is just selecting the Hcaldetid depth which is a statement that the HF_EM companion energy is from the HF_Had and vice versa.

I will change this to an if statement as there are only two possible comp values

const HcalDetId& detid = (HcalDetId)rechit.detId();
std::vector<uint32_t> RawDetIds;
Copy link
Contributor

Choose a reason for hiding this comment

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

use lower case initial for local variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this will be included in the next version

int heta = detid.ieta();
int hphi = detid.iphi();

if (std::abs(heta)>39) predphi=4;

int curphiL = hphi-predphi;
int curphiH = hphi+predphi;

if (predphi>abs(hphi) or predphi>abs(72-hphi))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this "72" magic number be get from some configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a product of the HF valid phi numbering range (1-72). I think this needs to be hardcoded as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let it hardcoded. But please extract it from the "HF valid phi numbering range", whenever it is stored/defined. So that if it gets ever modified you don't have to modify several hidden lines of code here and there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My frame of reference for this range (and most of these methods) is here

https://github.com/cms-sw/cmssw/blob/CMSSW_9_1_X/RecoLocalCalo/HcalRecAlgos/src/HcalHF_S9S1algorithm.cc

Here, the "magic" numbers 39,4,72 are not from an exterior definition. I will look further however

{
while (curphiL-predphi<0) curphiL+=72;
while (curphiH+predphi>72) curphiH-=72;
}

std::pair<std::vector<int>, std::vector<int>> phietas({heta,heta+1,heta-1,heta,heta},{hphi,hphi,hphi,curphiL,curphiH});
for(int in=0;in<5;in++)
{
HcalDetId TempID (HcalForward, phietas.first[in], phietas.second[in], comp);
RawDetIds.push_back(TempID.rawId());
}

for( const auto& jdx : ordered_hits )
{
const unsigned j = jdx;
const reco::PFRecHit& matchrechit = hits[j];
for( const auto& iID : RawDetIds ) if (iID==matchrechit.detId())compsumE+=matchrechit.energy();

}

}



const double rhenergy = rechit.energy();
// single spike cleaning
auto const & neighbours4 = rechit.neighbours4();
double surroundingEnergy = rechit.energy();
double surroundingEnergy = compsumE;//rechit.energy();
double neighbourEnergy = 0.0;
double layerEnergy = 0.0;
for( auto k : neighbours4 ) {
Expand Down
1 change: 1 addition & 0 deletions RecoParticleFlow/PFClusterProducer/src/SpikeAndDoubleSpikeCleaner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define __SpikeAndDoubleSpikeCleaner_H__

#include "RecoParticleFlow/PFClusterProducer/interface/RecHitTopologicalCleanerBase.h"
#include "DataFormats/HcalRecHit/interface/HFRecHit.h"

#include <unordered_map>

Expand Down