[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu #46489

artlbv · 2024-10-23T11:39:40Z

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.

…lues

cmsbuild · 2024-10-23T11:40:10Z

cms-bot internal usage

cmsbuild · 2024-10-23T11:42:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46489/42345

cmsbuild · 2024-10-23T11:43:05Z

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

cmsbuild · 2024-10-23T17:24:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46489/42349

rovere · 2024-10-24T06:15:16Z

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.

mmusich · 2024-10-24T06:49:49Z

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

mmusich · 2024-10-24T06:50:05Z

type bug-fix

aloeliger · 2024-10-28T10:40:23Z

L1Trigger/Phase2L1GT/python/menuConstants.py

+scalings_fname = environ["CMSSW_BASE"] + "/src/L1Trigger/Phase2L1GT/python/scalings_v44.json"
+with open(scalings_fname, 'r') as file:
+    scalings = json.load(file)


I don't think you are necessarily guaranteed this is always going to exist (with a partial checkout of CMSSW for example where you are not working with these directories directly).

The JSON itself should be committed as a data file as it is not directly code.

I'm not sure if there is a FileInPath solution for inside a python configuration, but maybe one of the core people know.

Should it be the RELEASE_BASE then instead?

On the separation to cms-data: I think that given these constants are tied to the L1 menu, it would make sense to keep them together imo. Otherwise it might be hard to disentangle if changes are due to the menu config or constant changes.

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.

@mmusich do you mean using this for example?

cmssw/Alignment/OfflineValidation/python/TkAlAllInOneTool/findAndChange.py

Line 55 in 902185e

_file = pfnInPath(path_to_check)

aloeliger · 2024-10-28T10:41:22Z

L1Trigger/Phase2L1GT/python/menuConstants.py

+objectIDs = {
+    "L1nnPuppiTau": {
+        "default": {
+            "qual" : 225
+        }
+    },
+    "L1tkPhoton":{
+        "Iso": {
+            "qual": {
+                "barrel": 0b0010,
+                "endcap": 0b0100,
+            },
+            "iso": {
+                "barrel": 0.25,
+                "endcap": 0.205,
+            }
+        }
+    },
+    "L1tkElectron":{
+        "Iso": {
+            # "qual": {
+            #     "barrel": 0b0010,
+            #     "endcap": 0b0010,
+            # },
+            "iso": {
+                "barrel": 0.13,
+                "endcap": 0.28,
+            }
+        },
+        "NoIso": {
+            "qual": {
+                "barrel": 0b0010,
+                "endcap": 0b0010,
+            },
+        },
+        "NoIsoLowPt": {
+            "qual": {
+                "barrel": 0b0010,
+                "endcap": 0b0000,
+            },
+        }
+    },
+    "L1gmtTkMuon":{
+        "VLoose": {
+            "qual": 0b0001,
+        },
+        "Loose": {
+            "qual": 0b0010,
+        },
+        "Medium": {
+            "qual": 0b0100,
+        },
+        "Tight": {
+            "qual": 0b1000,
+        },
+    }
+}


If we are already storing menu constants in JSON, could this also be added to the JSON?

Yes, I agree. though it would be a separate JSON, as the one with scalings is generated from the MenuTools.

gpetruc · 2024-10-29T07:08:55Z

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 <#46489 (comment)>: > +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 <#46489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDNWR7LCBTVNLSFZLZPDEDZ5Y4MDAVCNFSM6AAAAABQOTRMAOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJZGI4DGMZTGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

aloeliger · 2024-11-18T21:51:38Z

@artlbv what is that status of this PR?

HaarigerHarald · 2024-10-23T12:14:53Z

L1Trigger/Phase2L1GT/python/scalings_v44.json

Personally I'm not a big fan of adding a new file type. Also considering that the contents of that json is a perfectly valid python dict.

@artlbv Additional remark after discussion with Hannes, could we maybe sync up the object naming convention? It's super confusing for anyone reading the menu if in one place the object is called L1TrackJet and in another the collection is referred to as CL2JetsSC4 for example.

do you mean we should just save the JSON with .py as extension?

I agree and was planning to remap the object names in the JSON – will push this for this PR

Well you need a variable to assign the constant but other than that yes. Now, I understand that you had some automation with the json export in mind, but do you see the scales changing that often?

Great!

While at it we have some documentation in: https://github.com/cms-sw/cmssw/blob/master/L1Trigger/Phase2L1GT/README.md

Could you also add some documentation about the usage of scales when writing a menu?

HaarigerHarald · 2024-11-06T16:39:16Z

L1Trigger/Phase2L1GT/python/menuConstants.py

+    else:
+        return max(0, new_thr)
+
+def getObjectThrs(thr, obj, id):


@artlbv We think it would be better to explicitly write out the all thresholds instead of using this function that either returns a cms.double or cms.vdouble and hides what the regions actually mean. Hence we'd prefer

regionsMinPt = cms.vdouble(off2onl(52, "L1nnPuppiTau", "default", "barrel"), off2onl(52, "L1nnPuppiTau", "default", "endcap"))

this is too long and redundant imo (what if the off. threshold is mistyped in one of the regions?).
a compromise could be to list the regions as another argument, i.e.:

regionsMinPt = getObjectThrs(17, "L1nnPuppiTau", "default", ["barrel","endcap"])

how about this?

Looks good to me as well, since one can at least guess that this might return a cms.vdouble.

artlbv · 2024-11-19T18:55:18Z

@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?

HaarigerHarald · 2024-11-19T19:08:52Z

@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 #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.

artlbv · 2024-11-20T09:36:10Z

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

VourMa · 2024-11-20T10:06:39Z

@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.

cmsbuild · 2024-11-22T13:12:23Z

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.

mmusich · 2024-12-17T15:36:27Z

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 #46924). Can someone update on the foreseen trajectory for this PR from the L1 side @artlbv @cms-sw/l1-l2 ?

mmusich · 2024-12-17T15:36:45Z

@cmsbuild, please test

cmsbuild · 2024-12-17T17:19:13Z

+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: e2e4b6c
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

artlbv added 13 commits October 10, 2024 19:18

Update thresholds for DiTau52 seed in L1P2GT corresponding to 142x va…

6ee092d

…lues

Introducing scalings for P2GT as in the MenuTools

f45db12

Add object ID getter functions for P2GT menu for Photons

4e8522d

Add object ID and threshold getters for Ele seeds

ad00aa3

Add Obj getter for TkMuon base seeds

b41ce94

Return directly cms type from object getter

2a586eb

Add obj getter to xLep seeds

a83f4bd

Add obj getter to hadxLep seeds

0b37b83

More conversions to obj getter for P2GT

f32e2c6

Add ID lookup to multi muon seeds

325d4e7

Fix for threshold getter for single regions

edcb741

fix typos in P2GT triMu seed names

1bc8780

Fix thr option for MHT in P2GT

5e95741

cmsbuild added this to the CMSSW_14_2_X milestone Oct 23, 2024

cmsbuild added l1-pending pending-signatures tests-pending orp-pending upgrade-pending code-checks-pending labels Oct 23, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 23, 2024

Fix TripleMu names in L1P2GT

e2e4b6c

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 23, 2024

cmsbuild removed the code-checks-pending label Oct 23, 2024

cmsbuild added tests-approved and removed tests-started labels Oct 23, 2024

cmsbuild added the bug-fix label Oct 24, 2024

aloeliger reviewed Oct 28, 2024

View reviewed changes

This was referenced Nov 15, 2024

Export online thresholds from menu seeds cms-l1-dpg/Phase2-L1MenuTools#84

Open

Update P2GT emulator menu in CMSSW to AR24 cms-l1-dpg/Phase2-L1MenuTools#86

Closed

HaarigerHarald reviewed Nov 19, 2024

View reviewed changes

cmsbuild modified the milestones: CMSSW_14_2_X, CMSSW_15_0_X Nov 22, 2024

cmsbuild added tests-started and removed tests-approved labels Dec 17, 2024

cmsbuild added tests-approved and removed tests-started labels Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu #46489

[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu #46489

artlbv commented Oct 23, 2024

cmsbuild commented Oct 23, 2024 •

edited

Loading

cmsbuild commented Oct 23, 2024

cmsbuild commented Oct 23, 2024

cmsbuild commented Oct 23, 2024

rovere commented Oct 24, 2024

mmusich commented Oct 24, 2024

mmusich commented Oct 24, 2024

aloeliger Oct 28, 2024

artlbv Oct 28, 2024

mmusich Oct 28, 2024

artlbv Nov 19, 2024

aloeliger Oct 28, 2024

artlbv Oct 28, 2024

gpetruc commented Oct 29, 2024 via email

aloeliger commented Nov 18, 2024

HaarigerHarald Oct 23, 2024

HaarigerHarald Nov 6, 2024

artlbv Nov 19, 2024

HaarigerHarald Nov 20, 2024

HaarigerHarald Nov 20, 2024

HaarigerHarald Nov 6, 2024

artlbv Nov 19, 2024

HaarigerHarald Nov 20, 2024

artlbv commented Nov 19, 2024

HaarigerHarald commented Nov 19, 2024 •

edited

Loading

artlbv commented Nov 20, 2024

VourMa commented Nov 20, 2024

cmsbuild commented Nov 22, 2024

mmusich commented Dec 17, 2024

mmusich commented Dec 17, 2024

cmsbuild commented Dec 17, 2024

[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu #46489

Are you sure you want to change the base?

[Phase-2 L1] Incorporate object threshold scalings, ID and isolation via dictionaries into the P2GT menu #46489

Conversation

artlbv commented Oct 23, 2024

PR description:

PR validation:

cmsbuild commented Oct 23, 2024 • edited Loading

cmsbuild commented Oct 23, 2024

cmsbuild commented Oct 23, 2024

cmsbuild commented Oct 23, 2024

rovere commented Oct 24, 2024

mmusich commented Oct 24, 2024

Timing

Trigger Results

mmusich commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpetruc commented Oct 29, 2024 via email

aloeliger commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artlbv commented Nov 19, 2024

HaarigerHarald commented Nov 19, 2024 • edited Loading

artlbv commented Nov 20, 2024

VourMa commented Nov 20, 2024

cmsbuild commented Nov 22, 2024

mmusich commented Dec 17, 2024

mmusich commented Dec 17, 2024

cmsbuild commented Dec 17, 2024

Comparison Summary

cmsbuild commented Oct 23, 2024 •

edited

Loading

HaarigerHarald commented Nov 19, 2024 •

edited

Loading