Out-of-time photons in RECO/AOD #18844

kmcdermo · 2017-05-19T01:16:07Z

This PR is to include high energy out-of-time (OOT) photons in a dedication collection in AOD (reco::Photon), as well as the underlying objects (reco::PhotonCore, reco::SuperCluster, reco::CaloCluster). Any EcalRecHits added by the new clusters are added directly to the reduced collection. These collections are needed for the displaced photon and monophoton analysis groups.

This custom workflow goes through most of PF mustache photon sequence, starting at PFRecHits and removing the time cleaning, then to PFClusters, PFSuperClusters, and lastly straight from PFSuperClusters to photons.

At the PFSuperCluster producer, we filter on the seed crystal being flagged OOT to reduce duplication of clusters and superclusters. As such, this collection will only store photons whose seed is flagged OOT. Since we do not go through the full gedPhoton sequence, we do not do any PF isolations.

The increase to CPU is roughly 0.3%, while the increase in compressed size at AOD is about 1.5 kB from the new collections + additional rechits. Just taking an ls -l and dividing by the number of events processed, the estimated increase in size is 4.5 kB / event.

This work will be presented tomorrow at the EGM Reco meeting -- I will attach slides after the meeting.

cmsbuild · 2017-05-19T01:16:28Z

A new Pull Request was created by @kmcdermo (Kevin McDermott) for master.

It involves the following packages:

RecoEcal/Configuration
RecoEcal/EgammaClusterAlgos
RecoEcal/EgammaClusterProducers
RecoEgamma/Configuration
RecoEgamma/EgammaPhotonProducers
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @Sam-Harper, @rafaellopesdesa, @argiro, @cbernet, @lgray, @bachtis this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-05-19T04:58:18Z

@kmcdermo
please close this PR
It is apparently fully included and is as such superseded by #18845

kmcdermo · 2017-05-19T07:54:03Z

Hi @slava77 , so I opened this separately at the request of @arizzi, @gpetruc, @bendavid in the case that the details of the miniAOD workflow still need to be worked out separately -- in the meantime this can at least enter central reco while the details of slimming/etc are worked out.

I can of course close this, just let me know either way.

slava77 · 2017-05-19T11:49:00Z

On 5/19/17 12:54 AM, Kevin McDermott wrote: Hi @slava77 <https://github.com/slava77> , so I opened this separately at the request of @arizzi <https://github.com/arizzi>, @gpetruc <https://github.com/gpetruc>, @bendavid <https://github.com/bendavid> in the case that the details of the miniAOD workflow still need to be worked out separately -- in the meantime this can at least enter central reco while the details of slimming/etc are worked out. I can of course close this, just let me know either way.

Well, from the RECO POV, if you want this separately, then the second PR can not be reviewed until this goes in the release and if we need any change it will have to be kept in sync in case you want to keep it open for others to look at it.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18844 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbvoP2i2n66vEMFBmtRkk1nknYENvks5r7UqcgaJpZM4Nf-A8>.

kmcdermo · 2017-05-19T11:57:47Z

That is fine by me, for sure. I will be at the RECO meeting this afternoon (after the EGM RECO) and this can be discussed there which is the best way to proceed. I have no opinion on this either way.

slava77 · 2017-05-19T12:25:05Z

@cmsbuild please test

cmsbuild · 2017-05-19T12:25:22Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19989/console Started: 2017/05/19 14:27

cmsbuild · 2017-05-19T13:49:13Z

+1
Tested at: 06dc767
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18844/19989/summary.html

cmsbuild · 2017-05-19T13:49:17Z

Comparison job queued.

cmsbuild · 2017-05-19T15:44:05Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18844/19989/summary.html

Comparison Summary:

You potentially added 24 lines to the logs
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 1781 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1833901
DQMHistoTests: Total failures: 9328
DQMHistoTests: Total nulls: 831
DQMHistoTests: Total successes: 1823562
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

slava77 · 2017-05-26T16:58:16Z

RecoEcal/EgammaClusterProducers/python/particleFlowSuperClusterOOTECAL_cff.py

+## modification for Algo
+particleFlowSuperClusterOOTECAL.isOOTCollection = cms.bool(True)
+particleFlowSuperClusterOOTECAL.barrelRecHits = cms.InputTag("ecalRecHit","EcalRecHitsEB")
+particleFlowSuperClusterOOTECAL.endcapRecHits = cms.InputTag("ecalRecHit","EcalRecHitsEE")


I thought that the discussion in #18845 converged on using reducedEcalRecHit here

Well, again, it depends on where we want this to run. If we want it to run from the start of RECO, we run it with the full collection. If we want to run it after AOD, then we run it there.

The cost at running at AOD is that we would be losing some of our recHits, which is a further loss in resolution.

slava77 · 2017-05-26T17:02:24Z

RecoEcal/EgammaClusterProducers/src/PFECALSuperClusterProducer.cc

@@ -50,9 +50,12 @@ PFECALSuperClusterProducer::PFECALSuperClusterProducer(const edm::ParameterSet&
    iConfig.getUntrackedParameter<bool>("verbose",false);

  superClusterAlgo_.setUseRegression(iConfig.getParameter<bool>("useRegression")); 
-
+
+  isOOTCollection_ = (iConfig.existsAs<bool>("isOOTCollection") ? iConfig.getParameter<bool>("isOOTCollection") : false);


"iConfig.existsAs" is not needed if the value is already inserted in the fillDescriptions.
Please remove.

Okay, will do.

slava77 · 2017-05-26T17:07:24Z

RecoEgamma/EgammaPhotonProducers/python/mustacheOOTPhotonCore_cff.py

+mustacheOOTPhotonCore = photonCore.clone()
+mustacheOOTPhotonCore.scHybridBarrelProducer = cms.InputTag("particleFlowSuperClusterOOTECAL:particleFlowSuperClusterOOTECALBarrel")
+mustacheOOTPhotonCore.scIslandEndcapProducer = cms.InputTag("particleFlowSuperClusterOOTECAL:particleFlowSuperClusterOOTECALEndcapWithPreshower")
+mustacheOOTPhotonCore.conversionProducer = cms.InputTag("conversions")


is there a good reason to prefix "mustache" ?
is there a plan to add another type of OOT photons? (I thought no)

If you would prefer to drop it, and just name this as ootPhoton*, I can do that. There are no other plans to add any other OOT photon collection(s).

slava77 · 2017-05-26T17:18:26Z

RecoParticleFlow/PFClusterProducer/python/particleFlowRecHitOOTECAL_cff.py

+
+particleFlowOOTRecHitECAL = particleFlowRecHitECAL.clone()
+particleFlowOOTRecHitECAL.producers[0].qualityTests[1].timingCleaning = cms.bool(False)
+particleFlowOOTRecHitECAL.producers[1].qualityTests[1].timingCleaning = cms.bool(False)


this should have some protection that modifications are done correctly.
Simple access by index can easily fail or modify something unrelated.
The minimal change I can think of is to drop "cms.bool" so that this fails if it tries to modify a qualityTest without the timingCleaning parameter.
Additional checks will probably require consistency in inputs.

so something like:
particleFlowOOTRecHitECAL.producers[0].qualityTests[1].timingCleaning = False?

I agree accessing by index is not ideal... producers[0] = EB, producers[1] = EE, so I guess if we move to a unified EBEE clustering, this would be a problem.

slava77 · 2017-05-26T17:34:50Z

What is the plan for validation/DQM for all of the added sequences?
Is it even possible without special samples?

kmcdermo · 2017-05-26T19:10:10Z

Currently, there is no plan for validation/DQM. And indeed, the best way to validate would be with a signal sample (unless we want to validate with spikes from data or something).

I guess we could just simply make a plot of the OOT photons vs seed time with data as a starter. But pT/eta/phi would be hard without a signal sample.

…lation" This reverts commit 3fb364a.

slava77 · 2017-06-06T12:52:19Z

On 6/6/17 4:06 AM, Kevin McDermott wrote: We would still need to modify whatever producer takes the interesting Hcal/ES detIds to also include this new collection, and then unify it to drop any repeat detIds. I'm not sure I understand the last comment. What I mean is that, the detId producers I modified (and now are suggested to simply clone) produce only a vector of detIds, not a collection of recHits themselves. So whatever producer uses the vector of detIds to produce an output collection of Hcal / ES recHits would need the collection of detIds from the cloned detId producer as well as the original detIds from the original detId producer (as opposed to modifying the detid producer so that we do not have to change the recHit producer input).

Thank you for the clarification. This additional input to the rechit selector/producer is supported by its design. So, my earlier requests for changes still stand. I find it a bad pattern to hardcode an additional (and not fully equivalent) photon collection in parallel. Minimally, the existing photon inputs can be changed to a vector of photon inputs, although this is restrictive in case modified selections or inputs may be necessary for OOT or other future photons. It seems more practical to use the existing functionality of the detId selector module and use a clone to configure selection for OOT photons.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18844 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbgW0Prn8hsZuRFzZoW_A8P0cz41xks5sBTKvgaJpZM4Nf-A8>.

kmcdermo · 2017-06-06T12:58:45Z

Okay, I will make the changes as requested. I will revert those changes to make a clean break.

slava77 · 2017-06-06T13:26:37Z

On 6/6/17 5:58 AM, Kevin McDermott wrote: Okay, I will make the changes as requested. I will revert those changes to make a clean break.

Thank you. a rebase that removes the commits to be undone may be better, but I wouldn't push for it in this case if you are not familiar with the method.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18844 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbrKoyWPPSaEIy7hBcModdQeizY8pks5sBU0FgaJpZM4Nf-A8>.

cmsbuild · 2017-06-06T18:02:29Z

Pull request #18844 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

kmcdermo · 2017-06-06T18:03:22Z

@slava77 I made the changes you suggested, and tested them locally with runTheMatrix.py -l limited -i all, all passed.

slava77 · 2017-06-06T18:12:28Z

@cmsbuild please test

cmsbuild · 2017-06-06T18:12:47Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20376/console Started: 2017/06/06 20:13

cmsbuild · 2017-06-06T20:12:12Z

+1
Tested at: c728ced
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18844/20376/summary.html

cmsbuild · 2017-06-06T20:12:15Z

Comparison job queued.

cmsbuild · 2017-06-06T23:01:07Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18844/20376/summary.html

Comparison Summary:

You potentially added 17 lines to the logs
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 1697 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1837605
DQMHistoTests: Total failures: 37367
DQMHistoTests: Total nulls: 781
DQMHistoTests: Total successes: 1799284
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

slava77 · 2017-06-08T17:41:13Z

+1

for #18844 c728ced

changes are in line with the description and the follow up review: addition of out-of-time egamma collections up to photon reco to AOD.
jenkins tests pass and comparisons show small differences in reduced calo rechit collections corresponding to new added objects; no changes in miniAOD
local tests with several 2016 data workflows and with 2017 ttbar PU MC wf 10224 show roughly expected behavior:
- new collections appear in the outputs and seem to make sense in content
- CPU use is up by about 0.3% or even less
- output size increase is more consistent with ~0.1%: the total size on 200 events in data with non-empty outputs is in the range of 300 - 500 bytes per event using RECO file (this is not split). A larger size increase in AOD reported in the PR description is likely due to still sub-optimal compression and full-split output which has larger overhead on smaller files.

Here are several plots for MET2016B wf 136.725 (black is with this PR, despite the legend)

cmsbuild · 2017-06-08T17:41:31Z

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

davidlange6 · 2017-06-08T18:04:31Z

+1

out of time photons all the way to AOD

06dc767

cmsbuild added this to the CMSSW_9_2_X milestone May 19, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 19, 2017

kmcdermo mentioned this pull request May 19, 2017

Out of time photons in miniAOD #18845

Closed

cmsbuild added tests-started and removed tests-pending labels May 19, 2017

cmsbuild added tests-approved and removed tests-started labels May 19, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 19, 2017

slava77 reviewed May 26, 2017

View reviewed changes

Revert "include ecal and hcal interesting rechits from pf cluster iso…

b6cf23f

…lation" This reverts commit 3fb364a.

simple clones for interesting ES and HCAL recHit producers

c728ced

cmsbuild added tests-started and removed tests-pending labels Jun 6, 2017

cmsbuild added tests-approved and removed tests-started labels Jun 6, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jun 6, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 8, 2017

cmsbuild added orp-approved and removed orp-pending labels Jun 8, 2017

cmsbuild merged commit 806f747 into cms-sw:master Jun 8, 2017

kmcdermo mentioned this pull request Jun 13, 2017

Out-of-time photons for MiniAOD #19205

Merged

slava77 mentioned this pull request Dec 28, 2021

Small fix in ECAL recHit collection names in PFECALSuperClusterProducer #36592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Out-of-time photons in RECO/AOD #18844

Out-of-time photons in RECO/AOD #18844

kmcdermo commented May 19, 2017

cmsbuild commented May 19, 2017

slava77 commented May 19, 2017

kmcdermo commented May 19, 2017

slava77 commented May 19, 2017 via email

kmcdermo commented May 19, 2017

slava77 commented May 19, 2017

cmsbuild commented May 19, 2017 •

edited

Loading

cmsbuild commented May 19, 2017

cmsbuild commented May 19, 2017

cmsbuild commented May 19, 2017

slava77 May 26, 2017

kmcdermo May 26, 2017

slava77 May 26, 2017

kmcdermo May 26, 2017

slava77 May 26, 2017

kmcdermo May 26, 2017

slava77 May 26, 2017

kmcdermo May 26, 2017

slava77 commented May 26, 2017

kmcdermo commented May 26, 2017

slava77 commented Jun 6, 2017 via email

kmcdermo commented Jun 6, 2017

slava77 commented Jun 6, 2017 via email

cmsbuild commented Jun 6, 2017

kmcdermo commented Jun 6, 2017

slava77 commented Jun 6, 2017

cmsbuild commented Jun 6, 2017 •

edited

Loading

cmsbuild commented Jun 6, 2017

cmsbuild commented Jun 6, 2017

cmsbuild commented Jun 6, 2017

slava77 commented Jun 8, 2017

cmsbuild commented Jun 8, 2017

davidlange6 commented Jun 8, 2017

Out-of-time photons in RECO/AOD #18844

Out-of-time photons in RECO/AOD #18844

Conversation

kmcdermo commented May 19, 2017

cmsbuild commented May 19, 2017

slava77 commented May 19, 2017

kmcdermo commented May 19, 2017

slava77 commented May 19, 2017 via email

kmcdermo commented May 19, 2017

slava77 commented May 19, 2017

cmsbuild commented May 19, 2017 • edited Loading

cmsbuild commented May 19, 2017

cmsbuild commented May 19, 2017

cmsbuild commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented May 26, 2017

kmcdermo commented May 26, 2017

slava77 commented Jun 6, 2017 via email

kmcdermo commented Jun 6, 2017

slava77 commented Jun 6, 2017 via email

cmsbuild commented Jun 6, 2017

kmcdermo commented Jun 6, 2017

slava77 commented Jun 6, 2017

cmsbuild commented Jun 6, 2017 • edited Loading

cmsbuild commented Jun 6, 2017

cmsbuild commented Jun 6, 2017

cmsbuild commented Jun 6, 2017

slava77 commented Jun 8, 2017

cmsbuild commented Jun 8, 2017

davidlange6 commented Jun 8, 2017

cmsbuild commented May 19, 2017 •

edited

Loading

cmsbuild commented Jun 6, 2017 •

edited

Loading