-
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
ootPhotons for 80X Legacy reprocessing #19404
Conversation
A new Pull Request was created by @kmcdermo (Kevin McDermott) for master. It involves the following packages: PhysicsTools/PatAlgos @perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 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 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:
|
I made a simple validation of this sequence by re-processing some signal MC from AODSIM produced through the standard 80X chain, then switching to 92X for the PAT step using this era customization (GMSB, ctau = 3m). Without any selection on the leading photon, this is the leading photon seed time for each photon collection produced in the PAT step (slimmedPatPhotons from gedPhotons and slimmedOOTPATPhotons from ootPhotons): However, as we have seen before, when we include some selection on the photon (namely, implementing the cut based VID: https://twiki.cern.ch/twiki/bin/view/CMS/CutBasedPhotonIdentificationRun2), the standard collection loses its late time photons, in which case our ootPhotons kick in: I would say this validates this sequence. |
I guess rebase to the latest IB with 19205? should I make a separate PR for the changes requested for the ReducedEGProducer or commit them here? For the sake of the backport, it would probably be easier with a separate PR |
On 6/29/17 7:53 AM, Kevin McDermott wrote:
@slava77 <https://github.com/slava77>
I guess rebase to the latest IB with 19205? should I make a separate PR
for the changes requested for the ReducedEGProducer or commit them here?
For the sake of the backport, it would probably be easier with a separate PR
if after rebase this PR does not have to have changes in
ReducedEGProducer to handle legacy repro,
it should be OK to have fixes for ReducedEGProducer made in a separate PR.
If there is some coupling, that separate PR will delay this one further.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19404 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcboVEQz3r0nVFgQDTzYTiMhq16NMrks5sI7p4gaJpZM4OCQgL>.
|
Comparison is ready Comparison Summary:
|
ecalSeverityLevel, | ||
particleFlowRecHitPS, | ||
particleFlowClusterPS, | ||
ootPhotonTask, |
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.
if the ES and *PS modules are needed to make ootPhotonTask to work, they better be added to ootPhotonTask
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.
well, they are not needed in the standard reco, only in this special case.
or rather, they are already produced in the standard reco chain, and therefore available, whereas here they have to be reproduced.
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.
let me know if I should move these things to the ootPhotonTask
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.
It's a fair argument that ootPhotonTask needs these extra modules only in the legacy reminiAOD setup.
The modifier can be applied to ootPhotonTask where it's defined. This PAT producer file isn't supposed to know what else is needed logically upstream of making ootPhoton
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.
Okay, fair enough, I will make this change.
…luous modifiers for legacy repro on recHits
okay, I have made the changes as recommended, and the tests locally worked. |
@cmsbuild please test workflow 136.7611,1325.5 |
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. |
+1
|
merge |
Comparison is ready Comparison Summary:
|
This PR sits on top of PR #19205 (ootPhotons in miniAOD) with only one new commit for the legacy sequence.
I was able to get this to run locally and produce the ootPhoton collection from 80X AOD (2016G, singlephoton). However, after rebasing, I cannot complete the runTheMatrix.py -l limited -i all because of the same problems with #19205, namely some PhaseII sequence breaking the workflow.
I had to touch a number of files to accommodate the differences between the new 92X interfaces and what is available in the 80X AOD. A number of products have to be regenerated for this sequence to run (see the customization in the ootPhotonProducer_cff.py). I tried following what was done in #19304 for customizing the configuration, as well as following along #19264 for minimally changing any code.
EDIT: I forgot to mention I built this on top of an IB: CMSSW_9_2_X_2017-06-21-2300, so as to include those two PRs listed above.