-
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
era for HI pp reference run and relval wf #21122
Conversation
… a corresponding relval wf
The code-checks are being triggered in jenkins. |
+code-checks |
A new Pull Request was created by @ttrk (Kaya Tatar) for master. It involves the following packages: Configuration/DataProcessing @perrotta, @prebello, @kpedro88, @fabozzi, @cmsbuild, @franzoni, @slava77, @GurpreetSinghChahal, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
self.eras=Run2_2017_ppRef | ||
self.promptCustoms += [ 'Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2017_ppRef' ] | ||
self.expressCustoms += [ 'Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2017_ppRef' ] | ||
self.visCustoms += [ 'Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2017_ppRef' ] |
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's reuse the available customisePostEra_Run2_2017
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.
nm, I actually opted to make a new method for Run2_2017_pp_on_XeXe.
so, no change
|
||
_ecalClustersHI_ppRef = ecalClusters.copy() | ||
_ecalClustersHI_ppRef += islandBasicClusters | ||
ppRef_2017.toReplaceWith(ecalClusters, _ecalClustersHI_ppRef) |
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.
can this be simply the same as what's used above for pA_2016 etc?
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.
This new era does not need the full islandClusteringSequence, but only islandBasicClusters.
Your suggestion would also work, in that case the other modules in islandClusteringSequence would not be run, since their output will not be used.
I chose to make the customization only for those modules that are going to be run. That is why I made it separate.
What do you suggest ? If you want, I can move the modification and make it part of what is done before.
from Configuration.Eras.Modifier_ppRef_2017_cff import ppRef_2017 | ||
for ec in [RecoEgammaAOD.outputCommands, RecoEgammaRECO.outputCommands, RecoEgammaFEVT.outputCommands]: | ||
ppRef_2017.toModify( ec, func=lambda outputCommands: outputCommands.extend(['keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducerppGED_*_*', | ||
'keep recoHIPhotonIsolationedmValueMap_photonIsolationHIProducerpp_*_*' |
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.
is there an obvious need to be surgical here and not simply follow the same mods as for pA_2016 above?
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.
same as the previous comment. islandPhotons are not added by this. So I made a separate block. If you agree, then I can move it to the block of pA_2016.
@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:
|
The code-checks are being triggered in jenkins. |
+code-checks |
Pull request #21122 was updated. @perrotta, @prebello, @kpedro88, @fabozzi, @cmsbuild, @franzoni, @slava77, @GurpreetSinghChahal, @davidlange6 can you please check and sign again. |
@cmsbuild please test workflow 149.0 |
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:
|
+1
|
merge |
@@ -1569,6 +1575,8 @@ def gen2018HiMix(fragment,howMuch): | |||
steps['RECOHI2015']=merge([hiDefaults2015,{'-s':'RAW2DIGI,L1Reco,RECO,VALIDATION,DQM'},step3Up2015Defaults]) | |||
steps['RECOHI2011']=merge([hiDefaults2011,{'-s':'RAW2DIGI,L1Reco,RECO,VALIDATION,DQM'},step3Defaults]) | |||
|
|||
steps['RECOPPREF2017']=merge([ppRefDefaults2017,step3Up2015Defaults]) |
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.
I notice that in the 94X backport PR (#21069) this line reads
steps['RECOPPREF2017']=merge([ppRefDefaults2017,{'-s':'RAW2DIGI,L1Reco,RECO,EI,PAT,VALIDATION:@standardValidation+@miniAODValidation,DQM:@standardDQM+@miniAODDQM'},step3Up2015Defaults])
i.e. the RECOSIM got removed: I think the same should be also done here
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.
@perrotta
I don't understand your comment.
Please elaborate.
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.
While reviewing #21069 I noticed that this line differs from
https://github.com/cms-sw/cmssw/pull/21069/files#diff-819af2ec677272fddf1fef50e788a88bR1578
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.
I consider this PR the correct implementation #21122 (comment)
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.
Ok, thank you Slava. I supposed it was done on purpose, instead.
Then it has to be fixed in the backport: I will add a note o that PR
+1 |
adds "Run2_2017_ppRef" era to run customized reco for HI pp reference run
adds a corresponding relval wf (149)
runs and stores photonIsolationHIProducer objects using the era