cms-sw
[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu
PR description:
The idea of this PR is to allow for a more convenient organisation and book-keeping of the P2GT emulator menu configurations. Previously threshold changes due to object changes (so-called offline-online scalings) or ID/Iso WP changes had to be changed manually in every seed. With this PR dictionaries are introduced for common ID and Isolation Working Points as well as reading of the scalings (a MenuTools output) is implemented.
This addresses #46470 and was agreed to with the P2GT developer team @haarigerharald and @qvyz as presented here in slide 42.
FYI @VourMa @rovere @slaurila @robertjward @gpetruc
PR validation:
Tested to work with the standard recipe we (and HLT) are using in 1420pre1:
cmsDriver.py l1nanoPhase2 -s L1,L1TrackTrigger,L1P2GT,USER:PhysicsTools/L1Nano/l1tPh2Nano_cff.l1tPh2NanoTask --customise PhysicsTools/L1Nano/l1tPh2Nano_cff.addFullPh2L1Nano \
--conditions auto:phase2_realistic_T33 \
--geometry Extended2026D110 \
--era Phase2C17I13M9 \
--eventcontent NANOAOD \
--datatier GEN-SIM-DIGI-RAW-MINIAOD \
--customise SLHCUpgradeSimulations/Configuration/aging.customise_aging_1000,Configuration/DataProcessing/Utils.addMonitoring,L1Trigger/Configuration/customisePhase2TTOn110.customisePhase2TTOn110 \
--filein /store/mc/Phase2Spring24DIGIRECOMiniAOD/TT_TuneCP5_14TeV-powheg-pythia8/GEN-SIM-DIGI-RAW-MINIAOD/PU200_AllTP_140X_mcRun4_realistic_v4-v1/2560000/11d1f6f0-5f03-421e-90c7-b5815197fc85.root \
--fileout file:output_Phase2_L1T.root \
--python_filename rerunL1_cfg.py \
--inputCommands="keep *, drop l1tPFJets_*_*_*, drop l1tTrackerMuons_l1tTkMuonsGmt*_*_HLT" \
--mc \
-n 1000 --nThreads 4
A simple check was comparing the edmConfigDump outputs pre and post PR as there the actual thresholds, ID and Isolation values are expanded into plaintext. The results seem reasonable.
Note that also the IDs used for TkMuons have changed since since the AR24 there are now 4 WPs (compared to 1 before).
We will check the agreement with the MenuTools once we get more stats processed (on small stats the agreement is rather good), but I would like to start the PR and the reviewing process, eventually soliciting feedback from our SW experts on these changes.
Note that the code function names in the menuConstants.py could be cleaned up etc, but the mechanism should the final one.
+code-checks
Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46489/42345
A new Pull Request was created by @artlbv for master.
It involves the following packages:
- L1Trigger/Phase2L1GT (upgrade, l1)
@Moanwar, @aloeliger, @cmsbuild, @epalencia, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. @Martin-Grunewald, @missirol, @mmusich this is something you requested to watch as well. @antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.
cms-bot commands are listed here
+code-checks
Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46489/42349
Pull request #46489 was updated. @Moanwar, @aloeliger, @cmsbuild, @epalencia, @srimanob, @subirsarkar can you please check and sign again.
Does this incorporate the actual fix for
For example, pDoublePuppiTau52_52 in our measurements has ~3 times the rate of pSingleTkEle36
as discussed in https://github.com/cms-sw/cmssw/issues/46470?
Does this incorporate the actual fix for
For example, pDoublePuppiTau52_52 in our measurements has ~3 times the rate of pSingleTkEle36
as discussed in #46470?
Yes. At least the P2GT thresholds are now as in the MenuTools, and for the MenuTools we see similar rates to what we had in the AR24, see our recent presentation at the L1 DPG (linked above, slide 38). We will eventually also rerun the full MinBias but the good match of the MenuTools and P2GT emulator on 150 TTbar events makes me positive about a good agreement.
+1
Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d2424a/42363/summary.html
COMMIT: e2e4b6ceb091cd04e1f34af547d2b0173b148458
CMSSW: CMSSW_14_2_X_2024-10-23-1100/el8_amd64_gcc12
Additional Tests: HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46489/42363/install.sh to create a dev area with all the needed externals and cmssw changes.
- HLT P2 Timing: chart
Comparison Summary
Summary:
- You potentially added 3 lines to the logs
- Reco comparison results: 123 differences found in the comparisons
- DQMHistoTests: Total files compared: 46
- DQMHistoTests: Total histograms compared: 3566331
- DQMHistoTests: Total failures: 415
- DQMHistoTests: Total nulls: 0
- DQMHistoTests: Total successes: 3565896
- DQMHistoTests: Total skipped: 20
- DQMHistoTests: Total Missing objects: 0
- DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
- Checked 201 log files, 171 edm output root files, 46 DQM output files
- TriggerResults: found differences in 4 / 44 workflows
The performance aligns with the measurements I did privately using only the fix for the DiTau52 path: +10% speed-up and a visible reduction of events triggering HLT Tau paths.
Indeed, putting here some details for the record as the PR test results, alas, are not forever.
Timing
CMSSW_14_2_X_2024-10-23-1100 |
CMSSW_14_2_X_2024-10-23-1100 + this PR |
|---|---|
baseline: 5304.3 ms/ev | this PR: 4799.6 ms/ev | speedup gain -9.51%
Trigger Results
TriggerResults: found differences in 4 / 44 workflows
e.g. for workflow 29634.0
Found 10 matching events, out of which 6 have different HLT results
Events Accepted Gained Lost Other Trigger
10 6 - -1 - pDoublePuppiJet112_112
10 7 - -2 - pDoublePuppiTau52_52
10 4 - -2 - pNNPuppiTauPuppiMet_55_190
10 4 +1 - - pPuppiHT450
10 5 - -2 - pSinglePuppiJet230
10 0 - - ~2 HLT_AK4PFPuppiJet520
10 0 - - ~1 HLT_PFPuppiHT1070
10 1 - - ~1 HLT_DoublePFPuppiJets128_DoublePFPuppiBTagDeepCSV_2p4
10 2 - - ~1 HLT_DoublePFPuppiJets128_DoublePFPuppiBTagDeepFlavour_2p4
10 1 - - ~2 HLT_DoubleMediumChargedIsoPFTauHPS40_eta2p1
10 3 - - ~2 HLT_DoubleMediumDeepTauPFTauHPS35_eta2p1
Hi,
For the long term strategy of storing the L1T Data menu, it's to be defined but it's not going to be files in the release. However, for MCs where just a fixed menu is included, a file in the release is probably good enough (it has been so with HLT).
To be better defined later, e.g. if the Phase-2 L1T has an HLT-like structure then we might as well use the same tech to store and version data menus for both.
Giovanni
Il Lun 28 Ott 2024, 15:33 Marco Musich @.***> ha scritto:
@.**** commented on this pull request.
In L1Trigger/Phase2L1GT/python/menuConstants.py https://github.com/cms-sw/cmssw/pull/46489#discussion_r1819182491:
+scalings_fname = environ["CMSSW_BASE"] + "/src/L1Trigger/Phase2L1GT/python/scalings_v44.json" +with open(scalings_fname, 'r') as file:
- scalings = json.load(file)
I'm not sure if there is a FileInPath solution for inside a python configuration, but maybe one of the core people know.
there is pfnInPath.
Otherwise it might be hard to disentangle if changes are due to the menu config or constant changes.
I am not sure to see the issue. Can you elaborate? Having said that, the main draw-back of storing the configuration in cms-data is that a configuration change will require a full release as opposed to a patch-release. Having said that it's not clear to me what's the long term strategy for storing the phase-2 L1T menu, so it's hard to say if that is going to be an issue.
— Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/pull/46489#discussion_r1819182491, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDNWR7LCBTVNLSFZLZPDEDZ5Y4MDAVCNFSM6AAAAABQOTRMAOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJZGI4DGMZTGQ . You are receiving this because you were mentioned.Message ID: @.***>
@artlbv what is that status of this PR?
there were some more pending checks before finishing this PR (+feedback from the P2GT that just came): see this JIRA ticket – check discrepancies with MenuTools when both use TkPhotons (no standalone EG) – decide what to do with the dynamic ID of the low pt Muons
I'm not sure how urgent these checks are wrt to merging this PR into master tbh. @rovere @VourMa do you prefer the PR to go in as is or rather to have the above discrepancies sorted out before?
@artlbv what is that status of this PR?
there were some more pending checks before finishing this PR (+feedback from the P2GT that just came): see this JIRA ticket – check discrepancies with MenuTools when both use TkPhotons (no standalone EG) – decide what to do with the dynamic ID of the low pt Muons
I'm not sure how urgent these checks are wrt to merging this PR into master tbh. @rovere @VourMa do you prefer the PR to go in as is or rather to have the above discrepancies sorted out before?
The muon quality was already updated as part of https://github.com/cms-sw/cmssw/pull/45606. I ran over 10000 ttbar pre and post change to ensure that both cuts are equivalent. They might have diverged since though I did not check this.
The muon quality was already updated as part of #45606. I ran over 10000 ttbar pre and post change to ensure that both cuts are equivalent. They might have diverged since though I did not check this.
This is another thing. The dynamic ID I refer to is something implemented in the MenuTools, but not possible in the GT. We need a DPG decision on this @slaurila @folguera
@rovere @VourMa do you prefer the PR to go in as is or rather to have the above discrepancies sorted out before?
From my side, I think it's better to finalize this PR, as it takes care of the most important issues we have been seeing. Then, we can follow up with the remaining fixes in a separate PR, especially because I understand that, for these extra fixes, there may not be an immediate solution.
Milestone for this pull request has been moved to CMSSW_15_0_X. Please open a backport if it should also go in to CMSSW_14_2_X.
From my side, I think it's better to finalize this PR, as it takes care of the most important issues we have been seeing. Then, we can follow up with the remaining fixes in a separate PR, especially because I understand that, for these extra fixes, there may not be an immediate solution.
we just got reminded that currently we can't get sound estimates on the timing increase due to new path additions (e.g. in https://github.com/cms-sw/cmssw/pull/46924). Can someone update on the foreseen trajectory for this PR from the L1 side @artlbv @cms-sw/l1-l2 ?
+1
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d2424a/43505/summary.html
COMMIT: e2e4b6ceb091cd04e1f34af547d2b0173b148458
CMSSW: CMSSW_15_0_X_2024-12-17-1100/el8_amd64_gcc12
Additional Tests: HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46489/43505/install.sh to create a dev area with all the needed externals and cmssw changes.
- HLT P2 Timing: chart
Comparison Summary
Summary:
- You potentially removed 1 lines from the logs
- Reco comparison results: 120 differences found in the comparisons
- DQMHistoTests: Total files compared: 46
- DQMHistoTests: Total histograms compared: 3510017
- DQMHistoTests: Total failures: 373
- DQMHistoTests: Total nulls: 0
- DQMHistoTests: Total successes: 3509624
- DQMHistoTests: Total skipped: 20
- DQMHistoTests: Total Missing objects: 0
- DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
- Checked 202 log files, 172 edm output root files, 46 DQM output files
- TriggerResults: found differences in 4 / 44 workflows
Hi @artlbv, I was wondering whether it is still the plan to update this PR these days. The last 15_0_X pre-release deadline is on the 4th of February and, given that 15_0_X is a candidate for our Phase 2 HLT sample production, it is important to have this in by then (apart from it being relevant in all of our timing tests).
Hi all, I finally got to polish this PR.
Changes I made:
- Fix scalings path location – DONE (moved to dict in python, hence simple import – any name convention things I need to consider?)
- Convert json to python (dict) file – DONE
- Rename L1 vars to P2GT names in scalings file - DONE
Pending changes:
- Add docu to readme
- Add explicit eta region names in the object threshold getter function
I think it would be possible to merge this before the 15_0_0 release, I'll polish the rest until Monday.
+code-checks
Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46489/43327
- Found files with invalid states:
- L1Trigger/Phase2L1GT/python/scalings_v44.json:
- Added: f45db124d18326ac2db3fc53a345a094d3501867
- Deleted: 6897d0cb1d5cae2076065dce548d852b4c70914c
- L1Trigger/Phase2L1GT/python/scalings_v44.json:
Pull request #46489 was updated. @Moanwar, @aloeliger, @cmsbuild, @epalencia, @srimanob, @subirsarkar can you please check and sign again.