-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Hgg photons variables in nanoAODs #36526
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36526/27422
|
A new Pull Request was created by @maxgalli (Massimiliano Galli) for master. It involves the following packages:
@cmsbuild, @mariadalfonso, @gouskos, @fgolf can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
1e87168
to
24e81c8
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36526/27425
|
Pull request #36526 was updated. @cmsbuild, @mariadalfonso, @gouskos, @fgolf can you please check and sign again. |
@maxgalli please write a title more descriptive |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-af04ae/21332/summary.html Comparison SummarySummary:
|
https://cms-nanoaod-integration.web.cern.ch/integration/36526/data106Xul17v2_size_report.html#Photon |
@@ -207,12 +212,24 @@ def make_bitmapVID_docstring(id_modules_working_points_pset): | |||
mvaID_Fall17V1p1 = Var("userFloat('mvaID_Fall17V1p1')",float,doc="MVA ID score, Fall17V1p1",precision=10), | |||
mvaID_WP90 = Var("userInt('mvaID_WP90')",bool,doc="MVA ID WP90, Fall17V2"), | |||
mvaID_WP80 = Var("userInt('mvaID_WP80')",bool,doc="MVA ID WP80, Fall17V2"), | |||
pfPhoIso03 = Var("photonIso()",float,doc="PF absolute isolation dR=0.3, photon component (uncorrected)"), | |||
trkSumPtHollowConeDR03 = Var("trkSumPtHollowConeDR03()",float,doc="Sum of track pT in a hollow cone of outer radius, inner radius"), |
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.
"trkSumPtHollowConeDR03" add in the doc that this mimic the Run2 HLT trigger.
- What will be used for Run3 ?
- can you reduce the precision of this float ? as documented in the presentation this will not be used in any CQR/PhotonIDMVA
- what is the minimum pT used at HLT ?
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 we will use this for Run 3 as well, it's one of the variables we cut on during the diphoton preselection.
- Sure, what precision should I set?
- @simonepigazzini, @rchatter
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.
@lfinco Do you think we can switch safely to the offline (reco) isolation definition and drop the HLT-like ones?
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.
Cuts at HLT are at 30 and 22 GeV.
If with offline reco isolation you mean the pf photon isolation, the two might be quite different.
Given that with the preselection we want to be as close as possible to the trigger selection, using trkSumPtHollowConeDR03 would be in principle better. The usage of pfPhoIso03 instead might have an impact on the trigger efficiencies (which are computed on top of the preselection) and this impact should be evaluated.
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.
Track isolation at HLT should be close to charged hadron isolation offline. It would be useful to compare the two distributions (track iso offline and charged hadron iso), to see how close they are.
@@ -207,12 +212,24 @@ def make_bitmapVID_docstring(id_modules_working_points_pset): | |||
mvaID_Fall17V1p1 = Var("userFloat('mvaID_Fall17V1p1')",float,doc="MVA ID score, Fall17V1p1",precision=10), |
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.
to save space this float mvaID_Fall17V1p1 can be removed, and the cutBased_Fall17V1Bitmap as well
UL analysis should be already using the V2,
This is done already for the electron, see how is done 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.
I confirm that 94X-V2 ID is the correct one to use with UL, no need of V1 anymore
chargedHadronIso = Var("chargedHadronIso()",float,doc="charged hadron isolation with dxy,dz match to pv"), | ||
pfChargedIsoPFPV = Var("chargedHadronPFPVIso()",float,doc="PF absolute isolation dR=0.3, charged component (PF PV only)"), |
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.
The two variables were are probably fully redundant, we should verify and only have one.
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 based on the discussion in the mattermost group can I remove pfChargedIsoPFPV
?
24e81c8
to
958d826
Compare
-1 Failed Tests: RelVals RelVals-INPUT AddOn RelVals
Expand to see more relval errors ...
RelVals-INPUT
Expand to see more relval errors ...AddOn Tests
Expand to see more addon errors ... |
electronVeto = Var("passElectronVeto()",bool,doc="pass electron veto"), | ||
pixelSeed = Var("hasPixelSeed()",bool,doc="has pixel seed"), |
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 we improve the self documentation ? i.e. the doc string
electronVeto : to add "analysis not sensitive to electron -> photon fake rate"
pixelSeed: to add "analysis sensitive to electron -> photon fake rate"
see information in the wiki https://twiki.cern.ch/twiki/bin/viewauth/CMS/CutBasedPhotonIdentificationRun2
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-af04ae/22863/summary.html Comparison SummarySummary:
|
size increase on DoubleEG below 5%, 14 variable added (marked in red), more results see here (Automatic test report for 3668055) |
+xpog report of changes are here #36526 (comment) self documentation can be improved, but also outside this PR |
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 |
Following this request, we separate the PR in two. Here we thus add only the variable related to photons, while we keep the other one for beamspot and rho.