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.
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
[EGM] Eta-Extended-Electrons (EEE) #35309
Changes from 8 commits
41c6d3e
402514c
f922c7f
4c1ffb3
e3a299f
5f50bb0
2adf5a8
dbce374
d686af8
ffa1bfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -36,7 +36,8 @@ PFEGammaFilters::PFEGammaFilters(const edm::ParameterSet& cfg) | |||||
ele_noniso_mva_(cfg.getParameter<double>("electron_noniso_mvaCut")), | ||||||
ele_missinghits_(cfg.getParameter<unsigned int>("electron_missinghits")), | ||||||
ele_ecalDrivenHademPreselCut_(cfg.getParameter<double>("electron_ecalDrivenHademPreselCut")), | ||||||
ele_maxElePtForOnlyMVAPresel_(cfg.getParameter<double>("electron_maxElePtForOnlyMVAPresel")) { | ||||||
ele_maxElePtForOnlyMVAPresel_(cfg.getParameter<double>("electron_maxElePtForOnlyMVAPresel")), | ||||||
allowEEEinPF_(cfg.getParameter<bool>("allowEEEinPF")) { | ||||||
auto const& eleProtectionsForBadHcal = cfg.getParameter<edm::ParameterSet>("electron_protectionsForBadHcal"); | ||||||
auto const& eleProtectionsForJetMET = cfg.getParameter<edm::ParameterSet>("electron_protectionsForJetMET"); | ||||||
auto const& phoProtectionsForBadHcal = cfg.getParameter<edm::ParameterSet>("photon_protectionsForBadHcal"); | ||||||
|
@@ -164,6 +165,16 @@ bool PFEGammaFilters::passElectronSelection(const reco::GsfElectron& electron, | |||||
} | ||||||
} | ||||||
|
||||||
//TEMPORARY hack. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please clarify in the code comment: temporary until when, to be fixed by who? better yet, link a github issue in the comment. |
||||||
//Do not allow new EtaExtendedEle to enter PF, until ID, regression of EEEs are in place. | ||||||
if (!allowEEEinPF_) { | ||||||
int nHitGsf = electron.gsfTrack()->numberOfValidHits(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
double absEleEta = fabs(electron.eta()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if ((absEleEta > 2.5) && (nHitGsf < 5)) { | ||||||
passEleSelection = false; | ||||||
} | ||||||
} | ||||||
|
||||||
return passEleSelection && passGsfElePreSelWithOnlyConeHadem(electron); | ||||||
} | ||||||
|
||||||
|
@@ -323,6 +334,16 @@ bool PFEGammaFilters::isElectronSafeForJetMET(const reco::GsfElectron& electron, | |||||
isSafeForJetMET = false; | ||||||
} | ||||||
|
||||||
//TEMPORARY hack. | ||||||
//Do not allow new EtaExtendedEle to be SafeForJetMET, until ID, regression of EEEs are in place. | ||||||
if (!allowEEEinPF_) { | ||||||
int nHitGsf = electron.gsfTrack()->numberOfValidHits(); | ||||||
double absEleEta = fabs(electron.eta()); | ||||||
if ((absEleEta > 2.5) && (nHitGsf < 5)) { | ||||||
isSafeForJetMET = false; | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks like a duplication with respect to the new lines 170-176 in the same file. please write a small function that you can call in either place, to avoid code duplication. |
||||||
|
||||||
return isSafeForJetMET; | ||||||
} | ||||||
bool PFEGammaFilters::isPhotonSafeForJetMET(const reco::Photon& photon, const reco::PFCandidate& pfcand) const { | ||||||
|
@@ -411,6 +432,7 @@ void PFEGammaFilters::fillPSetDescription(edm::ParameterSetDescription& iDesc) { | |||||
iDesc.add<unsigned int>("electron_missinghits", 1); | ||||||
iDesc.add<double>("electron_ecalDrivenHademPreselCut", 0.15); | ||||||
iDesc.add<double>("electron_maxElePtForOnlyMVAPresel", 50.0); | ||||||
iDesc.add<bool>("allowEEEinPF", false); | ||||||
{ | ||||||
edm::ParameterSetDescription psd; | ||||||
psd.add<double>("maxNtracks", 3.0)->setComment("Max tracks pointing at Ele cluster"); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -28,6 +28,8 @@ namespace { | |||||||||
theNoOutliersBeginEnd(conf.getParameter<bool>("NoOutliersBeginEnd")), | ||||||||||
theMinDof(conf.getParameter<int>("MinDof")), | ||||||||||
theMinNumberOfHits(conf.getParameter<int>("MinNumberOfHits")), | ||||||||||
theMinNumberOfHitsHighEta(conf.getParameter<int>("MinNumberOfHitsHighEta")), | ||||||||||
theHighEtaSwitch(conf.getParameter<double>("HighEtaSwitch")), | ||||||||||
rejectTracksFlag(conf.getParameter<bool>("RejectTracks")), | ||||||||||
breakTrajWith2ConsecutiveMissing(conf.getParameter<bool>("BreakTrajWith2ConsecutiveMissing")), | ||||||||||
noInvalidHitsBeginEnd(conf.getParameter<bool>("NoInvalidHitsBeginEnd")) {} | ||||||||||
|
@@ -40,6 +42,8 @@ namespace { | |||||||||
int theMinDof; | ||||||||||
|
||||||||||
int theMinNumberOfHits; | ||||||||||
int theMinNumberOfHitsHighEta; | ||||||||||
double theHighEtaSwitch; | ||||||||||
bool rejectTracksFlag; | ||||||||||
bool breakTrajWith2ConsecutiveMissing; | ||||||||||
bool noInvalidHitsBeginEnd; | ||||||||||
|
@@ -62,6 +66,8 @@ namespace { | |||||||||
desc.add<int>("MinDof", 2); | ||||||||||
desc.add<bool>("NoOutliersBeginEnd", false); | ||||||||||
desc.add<int>("MinNumberOfHits", 5); | ||||||||||
desc.add<int>("MinNumberOfHitsHighEta", 5); | ||||||||||
desc.add<double>("HighEtaSwitch", 5.0); | ||||||||||
desc.add<bool>("RejectTracks", true); | ||||||||||
desc.add<bool>("BreakTrajWith2ConsecutiveMissing", true); | ||||||||||
desc.add<bool>("NoInvalidHitsBeginEnd", true); | ||||||||||
|
@@ -93,14 +99,25 @@ namespace { | |||||||||
: KFFittingSmootherParam(other), theFitter(aFitter.clone()), theSmoother(aSmoother.clone()) {} | ||||||||||
|
||||||||||
Trajectory smoothingStep(Trajectory&& fitted) const { | ||||||||||
double absTrajEta = 99.0; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
if (!fitted.empty()) { | ||||||||||
absTrajEta = fabs(fitted.lastMeasurement() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
.updatedState() | ||||||||||
.freeTrajectoryState() | ||||||||||
->momentum() | ||||||||||
.eta()); //needed for eta-extended electrons | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very expensive. |
||||||||||
} | ||||||||||
int thisHitCut = theMinNumberOfHits; | ||||||||||
if (absTrajEta > theHighEtaSwitch) | ||||||||||
thisHitCut = theMinNumberOfHitsHighEta; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
if (theEstimateCut > 0) { | ||||||||||
// remove "outlier" at the end of Traj | ||||||||||
while ( | ||||||||||
!fitted.empty() && fitted.foundHits() >= theMinNumberOfHits && | ||||||||||
!fitted.empty() && fitted.foundHits() >= thisHitCut && | ||||||||||
(!fitted.lastMeasurement().recHitR().isValid() || (fitted.lastMeasurement().recHitR().det() != nullptr && | ||||||||||
fitted.lastMeasurement().estimate() > theEstimateCut))) | ||||||||||
fitted.pop(); | ||||||||||
if (fitted.foundHits() < theMinNumberOfHits) | ||||||||||
if (fitted.foundHits() < thisHitCut) | ||||||||||
return Trajectory(); | ||||||||||
} | ||||||||||
return theSmoother->trajectory(fitted); | ||||||||||
|
@@ -199,11 +216,22 @@ namespace { | |||||||||
#endif | ||||||||||
|
||||||||||
bool hasNaN = false; | ||||||||||
if (!smoothed.isValid() || (hasNaN = !checkForNans(smoothed)) || (smoothed.foundHits() < theMinNumberOfHits)) { | ||||||||||
double absTrajEta = 99.0; | ||||||||||
if (smoothed.isValid()) { | ||||||||||
absTrajEta = fabs(smoothed.lastMeasurement() | ||||||||||
.updatedState() | ||||||||||
.freeTrajectoryState() | ||||||||||
->momentum() | ||||||||||
.eta()); //needed for eta-extended electrons | ||||||||||
} | ||||||||||
int thisHitCut = theMinNumberOfHits; | ||||||||||
if (absTrajEta > theHighEtaSwitch) | ||||||||||
thisHitCut = theMinNumberOfHitsHighEta; | ||||||||||
if (!smoothed.isValid() || (hasNaN = !checkForNans(smoothed)) || (smoothed.foundHits() < thisHitCut)) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like code duplication with respect to L102-112. can you make this a function? |
||||||||||
if (hasNaN) | ||||||||||
edm::LogWarning("TrackNaN") << "Track has NaN or the cov is not pos-definite"; | ||||||||||
if (smoothed.foundHits() < theMinNumberOfHits) | ||||||||||
LogTrace("TrackFitters") << "smoothed.foundHits()<theMinNumberOfHits"; | ||||||||||
if (smoothed.foundHits() < thisHitCut) | ||||||||||
LogTrace("TrackFitters") << "smoothed.foundHits()<thisHitCut"; | ||||||||||
DPRINT("TrackFitters") << "smoothed invalid => trajectory rejected with nhits/chi2 " << smoothed.foundHits() | ||||||||||
<< '/' << smoothed.chiSquared() << "\n"; | ||||||||||
if (rejectTracksFlag) { | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these added parameters are all in their respective fillDescriptions method, then this customisation would not be needed. Is this the case or could it be achieved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem straightforward to me, as fillDescriptions function is not present in one of the codes where we added new parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one? Could it be added there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this part can be deleted:
because
TrackingTools/TrackFitters/plugins/KFFittingSmootherESProducer.cc
already havefillDescriptions
, and our new parameters are in there.But the other part, the following, is needed:
because new parameters are added in
TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h
, and there is nofillDescriptions
. AddingfillDescriptions
in MinHitsTrajectoryFilter.cc doesn't solve the problem. It's not clear where we need to addfillDescriptions
to make it work.If possible, we would like to proceed with HLT customisation for now, and we will try to figure out
fillDescriptions
later on together with tracking experts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you then please remove the
'KFFittingSmootherESProducer'
customisation as it is not needed?Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, HLTrigger/Configuration/python/customizeHLTforCMSSW.py shows a merge conflict, so it needs to be updated anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad on the customisation @swagata87; when we discussed it offline, I missed that it was not needed for one of the modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now removed
KFFittingSmootherESProducer
customisation and resolved the conflict...oh no problem!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick look, it seems that the issue is the lack of a
fillDescriptions
in CkfTrackCandidateMaker (which in turn would probably be implemented as a function in CkfTrackCandidateMakerBase). That being said, this looks like something beyond the scope of this PR.