Skip to content
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

Put GED photons into HI event content and DQM pathways #10700

Merged

Conversation

harmonicoscillator
Copy link
Contributor

The Heavy Ion developers have decided to include the pp-style GED photons in the default HI RECO and DQM pathways. We plan to use GED photons as our main analysis workhorse for run2 data, using the legacy reconstruction as an aide in commissioning. Currently the GED photons are already run during RECO, but they are not saved in the event content or run through DQM. This PR adds them to the event content and the DQM pathways.

The first set of commissioning plots, comparing the legacy photon reco and the GED reco in HI MC can be found in this meeting:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/PhotonAnalyses2015#2015_08_11_13_30_CET

This PR should not change any pp workflows. We expect event size to only increase slightly (the photon collections are generally not large). Local tests show that the GED DQM plots show up properly under the egamma heading.

We also will backport to 75X for datataking. @yetkinyilmaz , @mandrenguyen , and @yenjie are probably interested in this.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @richard-cms (R. Alex Barbieri) for CMSSW_7_6_X.

Put GED photons into HI event content and DQM pathways

It involves the following packages:

DQMOffline/Configuration
RecoHI/Configuration
RecoHI/HiEgammaAlgos

@cmsbuild, @cvuosalo, @danduggan, @deguio, @slava77 can you please review it and eventually sign? Thanks.
@threus, @MiheeJo, @jazzitup, @echapon, @yenjie, @kurtejung, @istaslis, @rociovilar, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@@ -24,7 +26,9 @@
"drop recoPFClusters_*_*_*",
"keep recoElectronSeeds_*_*_*",
"keep recoGsfElectrons_*_*_*",
'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer_*_*'
'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer_*_*',
'keep recoPhotons_gedPhotonsTmp_*_*',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is relinking step added as well (the one that makes gedPhotons post-PF in pp reco)?
It's unfortunate to have these intermediate names in the HI output, coming with *Tmp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the relinking step is run. (There is no particleFlow collection, only particleFlowTmp.). I'm not too sure about the details of that, but I've never seen any non-tmp PF collections in HI reco.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have said "going to be added".
Is it intentional for the HI setup to use the *Tmp names? Unless there is a near term plan to add relinking, the current naming setup looks awkward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 "Intentional" is probably the wrong word, but up until now the PF in the heavyIon scenario only includes the sequences with 'Tmp' appended at the end. At the time this was set up, I understood that the rest of the PF sequence was to remove PU, which won't do anything in for the HI reco, as we only reconstruct a single PV.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 8/12/15 10:45 AM, mandrenguyen wrote:

In RecoHI/HiEgammaAlgos/python/RecoHiEgamma_EventContent_cff.py
#10700 (comment):

@@ -24,7 +26,9 @@
"drop recoPFClusters_____",
"keep recoElectronSeeds_____
",
"keep recoGsfElectrons_____*",

  • 'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer____'
  • 'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer____',
  • 'keep recoPhotons_gedPhotonsTmp____',

@slava77 https://github.com/slava77 "Intentional" is probably the
wrong word, but up until now the PF in the heavyIon scenario only
includes the sequences with 'Tmp' appended at the end. At the time this
was set up, I understood that the rest of the PF sequence was to remove
PU, which won't do anything in for the HI reco, as we only reconstruct a
single PV.

No, the rest of the PF sequence leading to products without Tmp
is not about PU removal.
It updates the refs pointing to PFCandidates and
adds PF-specific info as isolation or updates momentum based on PF
knowledge (for muons)


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10700/files#r36875840.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that sounds like something we should fix for the heavyIon scenario then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then staying with *Tmp now looks like the right strategy
[the relinking step will introduce new products reading these *Tmp
and making non-Tmp ones
]

On 8/12/15 10:55 AM, mandrenguyen wrote:

In RecoHI/HiEgammaAlgos/python/RecoHiEgamma_EventContent_cff.py
#10700 (comment):

@@ -24,7 +26,9 @@
"drop recoPFClusters_____",
"keep recoElectronSeeds_____
",
"keep recoGsfElectrons_____*",

  • 'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer____'
  • 'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducer____',
  • 'keep recoPhotons_gedPhotonsTmp____',

Ok, that sounds like something we should fix for the heavyIon scenario then.


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10700/files#r36877049.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, and when I add this relinking I will remove these 'keep' statements from the event content, as the non-Tmp collections will be saved, since the HI event content is a superset of the pp one.

@slava77
Copy link
Contributor

slava77 commented Aug 12, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #10700 c65b66f

Adding GED photons to the Heavy Ion event content.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-08-12-1100 show no significant differences. For workflow 140.53 with 70 events, the GED photons add 0.07% to the RECO output size.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2015

@cvuosalo
Is the CPU the same? I see there is some new module added

@deguio
Copy link
Contributor

deguio commented Aug 14, 2015

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@cvuosalo
Copy link
Contributor

@slava77:
The GED photons module seems to add about 95 ms/event, but the overall effect appears to be a wash:

Summary for 70 events
Baseline CMSSW_7_6_0_pre3
Time av 21.6739 s/evt   max 105.014 s on evt 38
M1 Time av 21.7052 s/evt   max 105.014 s on evt 38

PR 10700
Time av 21.6203 s/evt   max 105.591 s on evt 59
M1 Time av 21.6481 s/evt   max 105.591 s on evt 59

f1 is baseline, f2 is PR
Only f1 module count: 0
Only f2 module count: 1
    photonIsolationHIProducerGED     94.5796 ms/ev
     Total only f2: 94.5796
Common modules count (only significant diffs shown): 97
    hiDetachedTripletStepSeedLayers      1.17215 ms/ev -> 0.864716 ms/ev
    hiRegitMuInitialStepSelector     0.633827 ms/ev -> 0.940807 ms/ev
    calomuons    0.194066 ms/ev -> 0.120963 ms/ev
    electronGsfTracks    0.91104 ms/ev -> 1.17497 ms/ev
    particleFlowEGamma   212.566 ms/ev -> 189.546 ms/ev
    gedGsfElectronCores      0.208229 ms/ev -> 1.29648 ms/ev
     Total in detailed printout:     215.685 ms/ev -> 193.944 ms/ev      delta: -21.7407
 Total times: 10514.4 ms/ev -> 10440.3 ms/ev     delta: -74.1227

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Aug 15, 2015
Put GED photons into HI event content and DQM pathways
@cmsbuild cmsbuild merged commit 9580403 into cms-sw:CMSSW_7_6_X Aug 15, 2015
cmsbuild added a commit that referenced this pull request Aug 19, 2015
Put GED photons into HI event content and DQM pathways (backport 75X #10700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants