-
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
Adopt ChargedHadronPFTrackIsolationProducer for PFTICL candidates #32202
Adopt ChargedHadronPFTrackIsolationProducer for PFTICL candidates #32202
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32202/19942
|
A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master. It involves the following packages: RecoParticleFlow/PFProducer @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
So this doesn't make any difference to existing workflow as expected. |
+1
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
assign upgrade |
New categories assigned: upgrade @kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks |
kind reminder @cms-sw/upgrade-l2 |
I think we need this fix for TICL injection to PF work in 11_1 and 11_2 (not enabled by default, but it's being tested by various people) in regular offline reco setting (but probably not important for HLT workflow). So, this could be a candidate for backport to both 11_1 and 11_2. |
+upgrade |
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
Am I correct that, if we want this in 11_2, we need to make a backport request? |
@hatakeyamak indeed, it might be worth making a separate backport to 11_2_X |
ok. will do. |
PR description:
[address issues reported by HGCAL folks]
Update ChargedHadronPFTrackIsolationProducer so that we won’t get a crash even when --customise RecoHGCal/TICL/iterativeTICL_cff.injectTICLintoPF is specified. PF candidates from TICL don’t go through PFBlocks. This is generally fine, but when
rawECALEnergy
andrawHCALEnergy
are set to TICL PF candidates for JetMET to perform PF hadron calibration, it leads to a logic error. More in detail, after setting these quantities [2.1], theChargedHadronPFTrackIsolationProducer
[2.2] crashes because it checks the PFElement of typeTRACK
to identify isolated charged hadrons. (It was not crashing before as therawECALEnergy
andrawECALEnergy
are 0, effectively bypassing this check.)This isolation check is relevant for PF candidates coming from PFAlgo, where multiple tracks can associate to the same calorimeter clusters, but this is not relevant for PF candidates from TICL. In this PR, we consider PF charged candidates from TICL isolated, avoid the crash, and allow
rawCaloEnergy
&rawHcalEnergy
to be properly stored for packed candidates.[2.1] 1e3e2ec#diff-49ac77e92d14b3c79dfe08879ad1b7f4054eb34bf175e844010e4a77c0d242f1
[2.2] https://github.com/cms-sw/cmssw/blob/master/RecoParticleFlow/PFProducer/plugins/ChargedHadronPFTrackIsolationProducer.cc#L53-L77
PR validation:
Check in a manual workflow with --customise RecoHGCal/TICL/iterativeTICL_cff.injectTICLintoPF
that rawCaloEnergy & rawHcalEnergy are stored in packed charged candidates. Example:
(raw)CaloFraction() is ~1 by definition now, because PFTICL candidates don’t really use the track pt information yet, and it comes from hgcal measurements.
ptTrk is stored, so I think we have necessary information stored.
Also, make sure ttbar 2021 and 2026D49 (without injectTICLintoPF, 11634.0 & 23234.0) run without a crash.
if this PR is a backport please specify the original PR and why you need to backport that PR:
This is not a backport.
@hqucms @rovere @felicepantaleo @bendavid