-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[PPS] Timing detector preparation for 2021 conditions #30252
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30252/16151
|
A new Pull Request was created by @forthommel (Laurent Forthomme) for master. It involves the following packages: DataFormats/CTPPSDetId @perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@forthommel I think we are recommending the change from "CTPPS" to "PPS" for new names, so new files and variables should use "PPS". Existing names with "CTPPS" can be left unchanged. |
@mundim @forthommel The file |
Hi @cvuosalo , I agree, but I can't see where this rule is broken. |
Right, @wpcarvalho is working on the DD4Hep migration in parallel, see e.g. https://indico.cern.ch/event/922849/contributions/3877286/attachments/2046061/3427992/PPS_OffSoftMet__2020-05-27.pdf |
@jan-kaspar For the name change to "PPS", I was thinking of |
@cmsbuild please test |
The tests are being triggered in jenkins.
|
-1 Tested at: 01a20d4 CMSSW: CMSSW_11_2_X_2020-06-15-1100 I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following workflows: runTheMatrix-results/136.88811_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL/step2_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL.log136.731 step3 runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log136.793 step3 runTheMatrix-results/136.793_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017/step3_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017.log136.874 step3 runTheMatrix-results/136.874_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM/step3_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM.log140.56 step2 runTheMatrix-results/140.56_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18/step2_RunHI2018+RunHI2018+RECOHID18+HARVESTDHI18.log1306.0 step3 runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log1330.0 step3 runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM+NANOUP15/step3_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM+NANOUP15.log158.0 step3 runTheMatrix-results/158.0_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO/step3_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO.log10042.0 step3 runTheMatrix-results/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017+NanoFull_2017/step3_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017+NanoFull_2017.log10024.0 step3 runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017+NanoFull_2017/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017+NanoFull_2017.log25202.0 step3 runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25+NANOUP15_PU25/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25+NANOUP15_PU25.log10824.0 step3 runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018.log10224.0 step3 runTheMatrix-results/10224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU+NanoFull_2017PU/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU+NanoFull_2017PU.log11634.0 step3 runTheMatrix-results/11634.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021/step3_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021.log12434.0 step3 runTheMatrix-results/12434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023/step3_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023.log250202.181 step4 runTheMatrix-results/250202.181_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25/step4_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25.log
I found errors in the following addon tests: cmsDriver.py RelVal -s HLT:HIon,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run3_data_HIon --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run3_pp_on_PbPb --processName=HLTRECO --filein file:RelVal_Raw_HIon_DATA.root --fileout file:RelVal_Raw_HIon_DATA_HLT_RECO.root : FAILED - time: date Tue Jun 16 00:34:06 2020-date Tue Jun 16 00:26:39 2020 s - exit: 16640 |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30252/18382
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
@forthommel , @jan-kaspar , sorry for repeated question: two xml files from this PR are essential for Run-3 geometry? If yes, then I would propose, that the next PR will be limited only for Run-3 geometry. The rest may be proposed in a next-next PR. The reason - we need to finalize DB payload. |
I guess that especially the @ghugo83 can confirm, but her plans are sort of compatible with your suggestion, AFAIK. She already prepared a branch where other modifications (e.g. z sign inversion) are factored out to be deferred to a follow-up PR. |
Yes the branch is at: https://github.com/ghugo83/cmssw/compare/parallel_oldDD_DD4hep...ghugo83:PPS_2021_rebase?expand=1 So, do I understand correctly that even if these XMLs are not in the cmsExtended2021 scenario, they will still be put in Run 3 DB? |
In the Prod DB there is a PPS could have its own How should PPS function in a standard Run 3 MC workflow? How much PPS simulation and reconstruction is enabled in a standard workflow? |
The XML files listed in cmsExtended2021 go into the "Extended" |
Thanks @ghugo83 ! #31383 is merged now, so I guess we can go ahead. Actually, before opening the PR, we should take care of setting the "isRun2" flag correctly for the "from DB" scenario. We could use the |
Many thanks @cvuosalo
This sounds to me as a good solution for PPS simulation geometry - but this is in the hands of @mundim .
This sounds to me as a good solution for PPS reco geometry - no change wrt. the currently working solution.
In Run2 (since ~2016), PPS was fully included in reco workflows, but not in any simu wf. The full simulation of PPS is now being integrated in CMSSW. Therefore with a bit of optimism, my guess for Run3 is that PPS will be fully included in both reco and simu workflows. |
I guess that - with your advice - this should be a straight-forward step, shouldn't it? |
Ok the latest branch is at: |
Yep |
Haaa that's why, makes sense. |
Sth I don't like though, with this possibility of only grepping the Run2 scenarios, and not the Run3 ones, is it means isRun2 must be set to false by default? |
Just set isRun2 to false by default, after discussion with @forthommel |
I suggest the following
|
Ok I am fully done with https://github.com/ghugo83/cmssw/compare/parallel_oldDD_DD4hep...ghugo83:PPS_2021_rebase?expand=1 |
Perfect, exactly what is done now |
Please set the isRun2 flag correctly first! |
OK, I see it now here: https://github.com/ghugo83/cmssw/compare/parallel_oldDD_DD4hep...ghugo83:PPS_2021_rebase?expand=1#diff-3aff74325a83642694f2f94a497cb281R22 Thanks! |
@jan-kaspar yes it is done, sorry it is a commit I pushed 1 min ago :) |
Yes, we were writing in parallel. All good from my side. Thanks! |
Ok so now @forthommel just has to open the PR, after he checks overlaps in his XMLs. |
Ok great! Closing all my open windows, and switching to another topic then. |
PR description:
This PR prepares the ground for future developments in PPS timing detectors offline SW. It includes a new set of DDLs for 2021 scenario (new timing station), with its corresponding
cfi
-file for testing before integration to on/offline conditions DBs.The geometry parser is hence modified to "accept seamlessly" this additional station, better exploiting the hierarchy used in geometry definition.
As a side development, the track reconstruction plugin is adapted to dynamically create a track fitting algorithm for each new pot/station observed in the rechits stream (i.e. timing pots are not hardcoded anymore in plugin implementation).
Algorithms being stored as an associative container (single pot detid → algorithm) of this latter, a hash function is defined for the
CTPPSDetId
object used as an index.PR validation:
For the new station definition, a simple test can be used (with the new 2021 geometry scenario) to dump all sensors/readout channels positions. No problem observed for the additional station.
For the tracking plugin, ran over 8626 events covering 4 LS of run 319524 (2018C), no difference observed in any of "critical" attributes in track collection.
(black solid line is 11_2_X_2020-06-15-1100, red crosses include this PR)
Matrix tests ongoing.
if this PR is a backport please specify the original PR and why you need to backport that PR: N/A
Before submitting your pull requests, make sure you followed this checklist: