Add PF relinking for the HI reco sequence #19927
Conversation
|
A new Pull Request was created by @mandrenguyen for master. It involves the following packages: RecoHI/Configuration @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
On 7/26/17 8:41 AM, mandrenguyen wrote:
For now, both the "Tmp" and final PF candidates are stored, so the event
size will increase. Eventually the Tmp collections will be dropped, but
we would like to keep both temporarily for comparisons.
Isn't the only difference between Tmp and final PF candidates in the Ref
value that point to egamma and muons with filled PF info?
For just this, it doesn't make much sense to preserve the Tmp,
just check with this PR that kinematics and count is the same.
If there is something else to compare, do you have the tools for it already?
|
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@slava77 The PF candidates for the Tmp and final collection appear to have different values for photons and electrons. We'd like to study this a bit further to see how it affects heavy ion events. I think we have the tools, but we're still missing the samples, as the heavier events are no longer produced in relval due to memory constraints. We will look into it further with privately produced samples, which are anyway being produced for trigger studies. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| PFTowers.src = cms.InputTag("particleFlow") | ||
| akCs3PFJets.src = cms.InputTag("particleFlow") | ||
| akCs4PFJets.src = cms.InputTag("particleFlow") | ||
| kt4PFJetsForRho.src = cms.InputTag("particleFlow") |
There was a problem hiding this comment.
why is this all changed here instead of the places where the modules are defined
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #19927 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
On 8/3/17 2:52 AM, mandrenguyen wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In RecoHI/Configuration/python/Reconstruction_HI_cff.py
<#19927 (comment)>:
> @@ -35,6 +35,20 @@
from RecoLocalCalo.Configuration.hcalGlobalReco_cff import *
+#post PF egamma stuff
+from RecoHI.HiEgammaAlgos.HiEgammaPostPF_cff import *
+
+from RecoHI.HiJetAlgos.HiRecoPFJets_cff import *
+
+#reduced rechits
+from RecoEcal.EgammaClusterProducers.reducedRecHitsSequence_cff import *
+from RecoEcal.EgammaCoreTools.EcalNextToDeadChannelESProducer_cff import *
Presumably, the same reason it's needed here:
http://cmslxr.fnal.gov/source/RecoEcal/Configuration/python/RecoEcal_cff.py
which is to avoid the following:
Exception Message:
No "EcalNextToDeadChannelRcd" record found in the EventSetup for
synchronization value
Run: 1 LuminosityBlock: 1 Event: 0 Time: 1
Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception --
please move the ESProducer definition to the file which defines the
module or the sequence that needs it.
The top level config better be cleaner and not need to know about fine
grained details in subsequences
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19927 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbv39DNmT-ifGDCvse3t1f2ewueKgks5sUZhDgaJpZM4OkHB4>.
|
|
@slava77 So you'd like me to remove this import from Reconstruction_HI_cff.py, as well as RecoEcal.Configuration.RecoEcal_cff.py and place it directly in RecoEcal.EgammaClusterProducers.reducedRecHitsSequence_cff ? |
|
On 8/3/17 6:47 AM, mandrenguyen wrote:
@slava77 <https://github.com/slava77> So you'd like me to remove this
import from Reconstruction_HI_cff.py, as well as
RecoEcal.Configuration.RecoEcal_cff.py and place it directly in
RecoEcal.EgammaClusterProducers.reducedRecHitsSequence_cff ?
Uhm, I'm not sure that reducedRecHitsSequence is the only place where
this ESProducer is needed.
So, RecoEcal.Configuration.RecoEcal_cff perhaps should stay untouched
Your message in https://github.com/cms-sw/cmssw/pull/19927/files#r131099263
did not include the module which needed this ES product.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19927 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbnFR3gBzwd_CRTn9s6aP_kP4Alxbks5sUc-PgaJpZM4OkHB4>.
|
|
Ok, so I leave things as is then. Thanks. |
|
This is the CPU time taken by the additional modules, measured with wf 140.53 (re-reco of 2015 HI data). The extra time taken by those new modules alone is of the order of 3% of the previous total one |
|
About the event size, it increases overall by some 8.3%: 4397870 -> 4762106 4397870 -> 4762106 364236 8.3 ALL BRANCHES |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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. @davidlange6, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PF relinking was never included in the heavy-ion reco sequence. The logic was that was that the charged hadron subtraction for in-time PU isn't useful, since only a single PV is reco'd. However, it appears there are additional elements of egamma ID that come in at this stage. This PR reincludes the PF relinking with (only) the charged hadron subtraction explicitly disabled. With 1.5k events of wf 150.1, changes were observed to PF candidates of ID = 2 (electrons) and 4 (photons), but no changes to charged hadrons, neutral hadrons, or muons. For now, both the "Tmp" and final PF candidates are stored, so the event size will increase. Eventually the Tmp collections will be dropped, but we would like to keep both temporarily for comparisons.