-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Module for putting an invariant mass cut on the (mu,mu,gamma) triplet #37797
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37797/29708
|
A new Pull Request was created by @ats2008 (Aravind Sugunan) for master. It involves the following packages:
@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
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.
@ats2008 , a first set of comments from my side.
|
||
template <typename T1, typename T2, typename T3> | ||
HLTTripletMass<T1, T2, T3>::~HLTTripletMass() {} |
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.
template <typename T1, typename T2, typename T3> | |
HLTTripletMass<T1, T2, T3>::~HLTTripletMass() {} |
|
||
desc.add<bool>("is1and2Same", false); | ||
desc.add<bool>("is2and3Same", false); | ||
descriptions.add(defaultModuleLabel<HLTTripletMass<T1, T2, T3>>(), desc); |
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.
descriptions.add(defaultModuleLabel<HLTTripletMass<T1, T2, T3>>(), desc); | |
descriptions.addWithDefaultLabel(desc); |
// | ||
// Class imlements |dZ|<Max for a pair of two objects | ||
// |
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.
Update comment.
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.
Done !
filterproduct.addCollectionTag(originTag2_[i]); | ||
} | ||
tagOld = edm::InputTag(); | ||
for (trigger::size_type i3 = 0; i3 != n2; ++i3) { |
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.
for (trigger::size_type i3 = 0; i3 != n2; ++i3) { | |
for (trigger::size_type i2 = 0; i2 != n2; ++i2) { |
Just naming, but should be i2
, no?
const int triggerType3_; | ||
const std::vector<double> min_InvMass_; // minimum invariant mass of pair | ||
const std::vector<double> max_InvMass_; // maximum invariant mass of pair | ||
const double max_DR_; // maximum invariant mass of pair |
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.
Wrong comment.
for (; i3 != coll3.size(); i3++) { | ||
r3 = coll3[i3]; | ||
dauC_p4 = reco::Particle::LorentzVector(r3->px(), r3->py(), r3->pz(), r3->energy()); | ||
if (reco::deltaR2(dauAB_p4, dauC_p4) > max_DR_) { |
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.
Why cut on DeltaR^2
while the name of the parameter suggests this is DeltaR
?
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.
Ya ! that was a mistake , resolved ! thx
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37797/29759
|
Pull request #37797 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
adopted many suggestions from @missirol in the last commit. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37797/29760
|
Pull request #37797 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
Dear maintainers could you please review further and please try get this PR within the next pre-release ? @cms-bot urgent |
@ats2008 please merge the .h and .cc files into one (HLTTripletMass.cc) |
@ats2008 , I will complete the review of the PR later today. |
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.
@ats2008 , here's a 2nd round of review.
typedef HLTTripletMass<reco::RecoChargedCandidate, reco::RecoChargedCandidate, reco::RecoEcalCandidate> | ||
HLTMuMuPhotonMass; | ||
DEFINE_FWK_MODULE(HLTMuMuPhotonMass); |
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.
Once the h
and cc
files are merged, please move the instantiation to the file plugins/plugins.cc
.
Please use the name HLT3MuonMuonPhotonMass
, in analogy with existing plugins, e.g.
typedef HLTDoubletDZ<reco::Electron, reco::Electron> HLT2ElectronElectronDZ; |
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.
Done !
accept = accept and (n >= min_N_); | ||
return accept; |
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.
accept = accept and (n >= min_N_); | |
return accept; | |
return (accept and n >= min_N_); |
for (; i3 != coll3.size(); i3++) { | ||
r3 = coll3[i3]; | ||
dauC_p4 = reco::Particle::LorentzVector(r3->px(), r3->py(), r3->pz(), r3->energy()); | ||
if (reco::deltaR(dauAB_p4, dauC_p4) > max_DR_) { |
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 (reco::deltaR(dauAB_p4, dauC_p4) > max_DR_) { | |
if (reco::deltaR2(dauAB_p4, dauC_p4) > max_DR2_) { |
Use DeltaR-2 for the cut (faster).
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.
done !
accept = true; | ||
} | ||
|
||
if (accept) | ||
break; |
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.
accept = true; | |
} | |
if (accept) | |
break; | |
accept = true; | |
break; | |
} |
} | ||
} | ||
if (accept) | ||
break; |
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.
Why do break
in L209 and L213? Doesn't one want to find and count all the tri-candidates that pass the selection?
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.
yes , removing break
makes sense since min_N_
is also a parameter to the filter . I will update it
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.
done !
Pull request #37797 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
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 think a couple of include
s can be removed. @ats2008 , if you force-push again, please fix the commit message (right now, it's long and not very instructive).
#include "DataFormats/Common/interface/Handle.h" | ||
#include "DataFormats/Common/interface/Ref.h" | ||
#include "DataFormats/RecoCandidate/interface/RecoChargedCandidate.h" | ||
#include "DataFormats/RecoCandidate/interface/RecoChargedCandidateFwd.h" |
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.
#include "DataFormats/RecoCandidate/interface/RecoChargedCandidateFwd.h" |
#include "DataFormats/RecoCandidate/interface/RecoChargedCandidate.h" | ||
#include "DataFormats/RecoCandidate/interface/RecoChargedCandidateFwd.h" | ||
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidate.h" | ||
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h" |
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.
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h" |
Okay , did not see last comment by @missirol , will update |
f5692b3
to
b7c5b29
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37797/29903
|
Pull request #37797 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c26ef/24596/summary.html Comparison SummarySummary:
|
+hlt
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Towards the development of a new dedicated HLT Trigger for Bs->MuMuGamma.
Implementation modified from : HLTrigger/HLTfilters/plugins/HLTDoubletDZ.*
PR validation:
Setup
Get A proxy HLT Path
Get the customization code :
Add the customization lines to the end of
hltMC.py
. This will replacehltDisplacedDimuEG4DZFilter
with a module of typeHLTMuMuPhotonMass
added in the PRFor testing please run the hltMC.py and check the displayed Trigger results of the path
HLT_DoubleMu4_3_EG4_BsToMMG_v1
Trigger studies for the proposed trigger , in BPH Trigger Meeting.