Phase 2 fastHistoEmulation updated to 256 bins, with associated Track Selection changes #1130

NJManganelli · 2023-05-19T01:20:40Z

PR description:

This PR updates the Global Track Trigger fastHistoEmulation to match the current Firmware, as well as preparing the track selection and vertex association workflow changes for upcoming GTT PRs which can break other L1T workflows. The changes are detailed below.

fastHistoEmulation histogram updated from 128 bins to 256 bins
fastHistoEmulation track pt truncation removed in favor of truncating the sumPt of the final histogram bins
Track selection is split into separate TrackSelection (TS) and TrackVertexAssociation (TVA) modules, matching firmware architecture
VertexProducer is updated to ingest SelectedTracks from the TrackSelectionProducer module, required to match firmware
TrackJet module is updated to ingest Selected+VertexAssociated tracks, to better match firmware.
Default configurations for the TS and TVA for Jets and EtMiss are added, in addition to the TS used for the VertexProducer. This prepares CMSSW for future PRs to propagate the TrackJet's and EtMiss's internal/optimized TS+TVA into the standalone TS+TVA modules (centralizing the workflow-breaking changes)
the l1tVertexProducer_cfi is updated to include a default "l1tVertexFinder" and "l1tVertexFinderEmulator" matching typical usage in L1T workflows, removing need for configuring algo and inputs
other GTT module default configurations are updated such that they can be loaded and used without modification, simplifying workflows
L1TrackObjectNtupleMaker is updated to include indices for the separate TS and TVA tracks for VertexProducer, TrackJets, and EtMiss.
update to "l1t" naming convention in several places, for better consistency

Any workflow using the VertexProducer from L1Trigger.VertexFinder.l1tVertexProducer_cfi needs to be updated to also import the TrackSelection module, adding it to the tasks/sequence prior to the VertexProducer

from L1Trigger.L1TTrackMatch.l1tTrackSelectionProducer_cfi import *
tasklist.add(l1tTrackSelectionProducer)

DRAFT Updates

Changes are needed in locations where the VertexProducer is employed. Verification that the updates work and that the effects of fastHisto updates are negligible and/or acceptable in downstream locations (i.e. Level 1 Correlator) must be done, preferably by the relevant experts.

Disable the redundant TrackSelection for TrackJets (internal or external)
If needed, update createFirmwareInputFiles_cfg.py to re-enable the external TS for TrackJets (internal in passthrough mode)

Updates for workflows importing the VertexProducer

L1Trigger/L1TTrackMatch/python/L1TkObjectProducers_cff.py
L1Trigger/L1TTrackMatch/python/l1tTrackerEmuEtMiss_cfi.py
L1Trigger/L1TTrackMatch/test/L1TrackMET_cfg.py
L1Trigger/Phase2L1ParticleFlow/test/make_l1ctLayer1_dumpFiles_cfg.py
L1Trigger/Phase2L1ParticleFlow/test/make_l1ct_patternFiles_cfg.py
L1Trigger/Phase2L1Taus/python/l1emulator_cff.py

Sign-off for workflows importing the VertexProducer

L1Trigger/Configuration/python/SimL1Emulator_cff.py
L1Trigger/DemonstratorTools/test/gtt/createFirmwareInputFiles_cfg.py
L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker_cfg.py
L1Trigger/L1TTrackMatch/python/L1TkObjectProducers_cff.py
L1Trigger/L1TTrackMatch/python/l1tTrackerEmuEtMiss_cfi.py
L1Trigger/L1TTrackMatch/test/L1TrackMET_cfg.py
L1Trigger/Phase2L1ParticleFlow/test/make_l1ctLayer1_dumpFiles_cfg.py
L1Trigger/Phase2L1ParticleFlow/test/make_l1ct_patternFiles_cfg.py
L1Trigger/Phase2L1Taus/python/l1emulator_cff.py

PR validation:

scram code-check and scram code-format passed

Update Validation

validation of track selection and vertex finding, including significantly improved agreement with firmware
negligible change to vertex finding z position
L1Trigger/DemonstratorTools/test/gtt/createFirmwareInputFiles_cfg.py used for creating firmware/emulation test vectors for APx format
L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc used for testing ntuplization and effect on vertex finding

aloeliger · 2023-05-24T09:47:25Z

Hi @NJManganelli, I'm taking a look at this for the first time, and it's already huge. We've received requests/advice from the software groups that we should be breaking PRs like this up into more manageable chunks for review. Are there elements of this that are conceptually/software complete (if not performance complete) that can be split off for PR to central CMSSW?

aloeliger · 2023-05-24T16:03:50Z

@epalencia @gpetruc @EmyrClement @mcepeda @jheikkil @qvyz @artlbv after the talk in our most recent L1 Offline SW meeting, it has become apparent this is under tight time constraint, and input from correlator, GT, Menu and software experts is requested ASAP to check the suitability and integration impact of this PR. The hoped for timescale is ~3 weeks for L1 review of the impact, and for readiness for PR to central CMSSW. My understanding is that there is some slight give on these deadlines, but a firmware engineer is being lost so major changes after that time will be painful at best.

aloeliger · 2023-05-24T16:06:38Z

@mcepeda I am the last person who should be talking about paying attention to email, and sending email, but I would remind you that you agreed to email with Cristina and several people mentioned here to discuss the impact of this change.

gpetruc · 2023-05-24T16:28:31Z

@epalencia @gpetruc @EmyrClement @mcepeda @jheikkil @qvyz @artlbv after the talk in our most recent L1 Offline SW meeting, it has become apparent this is under tight time constraint, and input from correlator, GT, Menu and software experts is requested ASAP to check the suitability and integration impact of this PR.

OK, I haven't been looking at it nor trying run the correlator emulation on top of it so far since it looked an early draft. @NJManganelli @aloeliger if it's ready for either purpose let me know.

NJManganelli · 2023-05-24T18:28:01Z

@gpetruc This is ready for studies of any potential downstream effects in Correlator/GT/L1Menu

A clarification on track selection: this is the GTT internal track selection, which AFAIK is only used in VertexFinding and Track-Only GTT objects. Additionally, the track selection is (at the moment) very minimal, so significant changes are not anticipated

If for some reason the previous behavior of VertexFinding running on unselected tracks (essentially as GTT receives them from TFPs), this can be recovered by configuring the TrackSelection as is used for TrackJets in this PR:

cmssw/L1Trigger/L1TTrackMatch/python/l1tTrackSelectionProducer_cfi.py

Lines 40 to 52 in c546ca7

    
           l1tTrackSelectionProducerForJets = l1tTrackSelectionProducer.clone( 
        
             cutSet = dict( 
        
                               ptMin = 0.0, # pt must be greater than this value, [GeV] 
        
                               absEtaMax = 999.9, # absolute value of eta must be less than this value 
        
                               absZ0Max = 999.9, # z0 must be less than this value, [cm] 
        
                               nStubsMin = 0, # number of stubs must be greater than or equal to this value 
        
                               nPSStubsMin = 0, # the number of stubs in the PS Modules must be greater than or equal to this value 
        
                               reducedBendChi2Max = 999.9, # bend chi2 must be less than this value 
        
                               reducedChi2RZMax = 999.9, # chi2rz/dof must be less than this value 
        
                               reducedChi2RPhiMax = 999.9, # chi2rphi/dof must be less than this value 
        
                               ), 
        
           )

A change which cannot be reversed are those associated with the internal FastHisto update. The core of this is moving the histogramming from 128 bins to 256 bins (with identical range). The effect on actual VertexFinding is minimal, because the VF algorithm uses a pt-weighted sliding-window to find the vertex, and thus the actual change to the Primary Vertex position is very small and well within the previous version's resolution. As soon as I can, I will perform another validation of this on the current PR (the previous one having been done in CMSSW master) and send the results via email.

Here is a recipe for installation of this PR, which compiles and runs for GTT workflows, and successfully ran a runTheMatrix workflow including the SimL1Emulator_cff.py. (The full runTheMatrix is in progress).

#export SCRAM_ARCH=el8_amd64_gcc10 or export SCRAM_ARCH=slc7_amd64_gcc10
cmsrel CMSSW_12_5_2_patch1
cd CMSSW_12_5_2_patch1/src
cmsenv
git cms-init
git cms-addpkg DataFormats/StdDictionaries L1Trigger/L1TTrackMatch L1Trigger/TrackFindingTracklet DataFormats/L1TrackTrigger L1Trigger/DemonstratorTools L1Trigger/VertexFinder DataFormats/L1Trigger
git remote add cms-l1t-offline [email protected]:cms-l1t-offline/cmssw.git
git fetch cms-l1t-offline pull/1130/head:fast-histo-pr-1130
git checkout fast-histo-pr-1130
scram b clean && scram build -j8

aloeliger · 2023-05-26T09:44:24Z

L1Trigger/L1TTrackMatch/plugins/L1TrackVertexAssociationProducer.cc

+  enum TrackBitWidths {
+    kPtSize = TTTrack_TrackWord::TrackBitWidths::kRinvSize - 1,  // Width of pt
+    kPtMagSize = 9,                                              // Width of pt magnitude (unsigned)
+    kEtaSize = TTTrack_TrackWord::TrackBitWidths::kTanlSize,     // Width of eta
+    kEtaMagSize = 3,                                             // Width of eta magnitude (signed)
+  };


Would these be better as namespaced const's or constexpr's? Having them be an enumerated type seems a little strange for something that is just meant to be a set of constants

These are directly from L1TrackSelectionProducer.cc (from which the TVA Producer has been split off to match the firmware architecture). These also mirror the storage of these constants in the firmware, and across all these GTT modules in CMSSW, which makes me hesitant to change the format in only one place. At some point, consolidating these enum constants and potentially changing how they're stored should indeed be considered (because it's very WET when we should be DRY). However, some discussion and approval in GTT/L1T should happen first, I think

aloeliger · 2023-05-26T09:56:12Z

L1Trigger/L1TTrackMatch/plugins/L1TrackVertexAssociationProducer.cc

+    : l1VerticesToken_(iConfig.getParameter<bool>("processSimulatedTracks")
+                           ? consumes<l1t::VertexCollection>(iConfig.getParameter<edm::InputTag>("l1VerticesInputTag"))
+                           : edm::EDGetTokenT<l1t::VertexCollection>()),


if this takes a default empty value, wouldn't it be better to simply mark it as such in the fillDescriptions?

Perhaps I am reading this wrong, but processSimulatedTracks and processEmulatedTracks have defaults (to true), and this (by default) means that one tries to read from these input tags, but these input tags are optional. Isn't entirely possible to misconfigure this module quite easily by selecting yes to processing one of these track types, but forgetting to specify the input tag, in which case it will crash out trying to read parameters that don't exist?

A better solution may be to provide the default empty, and then throw an error if the corresponding boolean flag is available, but the collection is empty.

I think this line of code is correct: the call to consumes<...> has to be done only if processSimulatedTracks is true.
What's missing in the fillDescriptions is something that tells that l1VerticesInputTag should be set if processSimulatedTracks, which could be done with something like https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#IfExists

I've made the input tags non-optional, as Matti said ifExists isn't preferred here (switch could be used, but also, the convention has been to not introduce so much complexity in the description). Combined with the ternary consumes, if the tags aren't specified, they'll just have the default and won't cause an error if they don't exist.

aloeliger · 2023-05-26T09:56:26Z

L1Trigger/L1TTrackMatch/plugins/L1TrackVertexAssociationProducer.cc

+          iConfig.getParameter<bool>("processSimulatedTracks")
+              ? consumes<TTTrackRefCollectionType>(iConfig.getParameter<edm::InputTag>("l1SelectedTracksInputTag"))
+              : edm::EDGetTokenT<TTTrackRefCollectionType>()),


Same thing here

aloeliger · 2023-05-26T09:56:37Z

L1Trigger/L1TTrackMatch/plugins/L1TrackVertexAssociationProducer.cc

+          iConfig.getParameter<bool>("processEmulatedTracks")
+              ? consumes<l1t::VertexWordCollection>(iConfig.getParameter<edm::InputTag>("l1VerticesEmulationInputTag"))
+              : edm::EDGetTokenT<l1t::VertexWordCollection>()),


Same thing here

aloeliger · 2023-05-26T09:56:47Z

L1Trigger/L1TTrackMatch/plugins/L1TrackVertexAssociationProducer.cc

+                                          ? consumes<TTTrackRefCollectionType>(iConfig.getParameter<edm::InputTag>(
+                                                "l1SelectedTracksEmulationInputTag"))
+                                          : edm::EDGetTokenT<TTTrackRefCollectionType>()),


aloeliger · 2023-05-26T11:57:22Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

+  m_trk_selected_foretmiss_index = new std::vector<int>;
+  m_trk_selected_emulation_foretmiss_index = new std::vector<int>;
+  m_trk_selected_associated_foretmiss_index = new std::vector<int>;
+  m_trk_selected_associated_emulation_foretmiss_index = new std::vector<int>;



I don't see a corresponding delete for this. This leaks memory pretty badly. Every vector in here does. I get that this is some test module, but if this sees use outside private testing, and is going to end up in CMSSW, it would be better if it didn't leak. One could conceivable also explore the use of std::unique_ptr's here (or shared or whatever).

Thanks for catching this, indeed bare pointers are awful. Strangely none of the others in the ntupler are cleaned up either, so I'll add deletion of those in the endJob. I feel like I may be overlooking something else here, but I'm not really a user/maintainer on this bit of code.

aloeliger · 2023-05-26T11:57:33Z

L1Trigger/L1TTrackMatch/test/L1TrackObjectNtupleMaker.cc

+  m_trkExt_selected_associated_index = new std::vector<int>;
+  m_trkExt_selected_associated_emulation_index = new std::vector<int>;
+  m_trkExt_selected_forjets_index = new std::vector<int>;
+  m_trkExt_selected_emulation_forjets_index = new std::vector<int>;
+  m_trkExt_selected_associated_forjets_index = new std::vector<int>;
+  m_trkExt_selected_associated_emulation_forjets_index = new std::vector<int>;
+  m_trkExt_selected_foretmiss_index = new std::vector<int>;
+  m_trkExt_selected_emulation_foretmiss_index = new std::vector<int>;
+  m_trkExt_selected_associated_foretmiss_index = new std::vector<int>;
+  m_trkExt_selected_associated_emulation_foretmiss_index = new std::vector<int>;


These also leak

aloeliger · 2023-05-26T12:01:03Z

L1Trigger/VertexFinder/src/VertexFinder.cc

+      kBinSize = 8,                     // Width of a single bin in z
+      kBinFixedSize = 8,                // Width of a single z0 bin in fixed point representation
+      kBinFixedMagSize = 5,             // Width (magnitude) of a single z0 bin in fixed point representation


Again, I'm not sure I understand the need for the enumerated type. Is there some function of the enum that I am not seeing or remembering that could not be accomplished via some const or constexpr?

in past versions of c++ it used to be the good way to define compile-time integer constants, but it's true that since c++11 using constexpr its better

As above, this should probably be addressed simultaneously across all the GTT emulator (CMSSW) and firmware (HLS) code at once, following consensus. Keeping things uniform (or close to it) between CMSSW and HLS, which live in entirely different repos, makes maintenance easier.

aloeliger · 2023-05-26T12:04:52Z

L1Trigger/VertexFinder/src/VertexFinder.cc

@@ -986,29 +997,46 @@ namespace l1tVertexFinder {
                                         << "\n"
                                         << "tkZ0 = " << tkZ0.to_double() << "(" << tkZ0.to_string(2)
                                         << ")\ttkpt = " << tkpt.to_double() << "(" << tkpt.to_string(2)
-                                         << ")\tpt_tmp = " << pt_tmp << "\tbin = " << bin.first.to_int() << "\n"
+                                         << ")\tbin = " << bin.first.to_int() << "\n"


I would say that these LogInfo instances should be under LogDebug, but given that they are controlled by a debug parameter and not all of it is introduced here, it is not urgent.

This LogInfo has been used pretty frequently by the previous developers. I have no opposition to changing it (off the top of my head), but like the constants, should probably be coordinated across all the GTT modules at once.

aloeliger · 2023-05-26T12:05:12Z

L1Trigger/VertexFinder/src/VertexFinder.cc

+        edm::LogInfo("VertexProducer") << "fastHistoEmulation::truncating histogram bin pt once filling is complete \n"
+                                       << "hist_untruncated.at(" << hb << ") = " << hist_untruncated.at(hb).to_double()
+                                       << "(" << hist_untruncated.at(hb).to_string(2)
+                                       << ")\tbin_trunc = " << bin_trunc.to_double() << "(" << bin_trunc.to_string(2)
+                                       << ")\n\thist.at(" << hb << ") = " << hist.at(hb).to_double() << "("
+                                       << hist.at(hb).to_string(2) << ")";
+      }
+    }


here as well.

gpetruc · 2023-05-26T15:25:45Z

Hi, I had a quick look at the code too. I agree with Andrew that when trying to push to central cmssw reviewers will scream because of the bare c++ pointers in the ntuplizer, though there's no easy fix since the TTree::Branch API you're using wants the address to a pointer to a stl collection and so it might not work with unique_ptr. An option could be to keep the vectors as datamembers by value, and use the TTree Branch API for C arrays, but you'd need to book the variables for the sizes of the arrays and re-set the branch address since the vector may reallocate on resize. Or, write the arrays into a FlatTable in the EDM event and have a NanoAODOutputModule turn it into a TTree (a lot of code changes, though with the upside that dumping other objects in the ntuple could be easier if there's already a NanoAOD-compatible way of writing them) Il Mer 24 Mag 2023, 20:28 Nick ***@***.***> ha scritto:

…

@gpetruc <https://github.com/gpetruc> This is ready for studies of any potential downstream effects in Correlator/GT/L1Menu A clarification on track selection: this is the GTT internal track selection, which AFAIK is only used in VertexFinding and Track-Only GTT objects. Additionally, the track selection is (at the moment) very minimal, so significant changes are not anticipated If for some reason the previous behavior of VertexFinding running on unselected tracks (essentially as GTT receives them from TFPs), this can be recovered by configuring the TrackSelection as is used for TrackJets in this PR: https://github.com/cms-l1t-offline/cmssw/blob/c546ca7f87dd1dc263290b3409cb99b532c43f7e/L1Trigger/L1TTrackMatch/python/l1tTrackSelectionProducer_cfi.py#L40-L52 A change which cannot be reversed are those associated with the internal FastHisto update. The core of this is moving the histogramming from 128 bins to 256 bins (with identical range). The effect on actual VertexFinding is minimal, because the VF algorithm uses a pt-weighted sliding-window to find the vertex, and thus the actual change to the Primary Vertex position is very small and well within the previous version's resolution. As soon as I can, I will perform another validation of this on the current PR (the previous one having been done in CMSSW master) and send the results via email. Here is a recipe for installation of this PR, which compiles and runs for GTT workflows, and successfully ran a runTheMatrix workflow including the SimL1Emulator_cff.py. (The full runTheMatrix is in progress). #export SCRAM_ARCH=el8_amd64_gcc10 or export SCRAM_ARCH=slc7_amd64_gcc10 cmsrel CMSSW_12_5_2_patch1 cd CMSSW_12_5_2_patch1/src cmsenv git cms-init git cms-addpkg DataFormats/StdDictionaries L1Trigger/L1TTrackMatch L1Trigger/TrackFindingTracklet DataFormats/L1TrackTrigger L1Trigger/DemonstratorTools L1Trigger/VertexFinder DataFormats/L1Trigger git remote add cms-l1t-offline ***@***.***:cms-l1t-offline/cmssw.git git fetch cms-l1t-offline pull/1130/head:fast-histo-pr-1130 git checkout fast-histo-pr-1130 scram b clean && scram build -j8 — Reply to this email directly, view it on GitHub <#1130 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDNWR3THWWQHKBCZJHXS2LXHZHL3ANCNFSM6AAAAAAYHEXF3I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

NJManganelli · 2023-06-09T16:29:15Z

A set of updated summary slides are available at
https://indico.cern.ch/event/1294377/contributions/5446490/attachments/2663681/4615224/NickManganelli_FastHistoUpdate256TS.pdf

BenjaminRS · 2023-06-10T15:38:54Z

I have performed a test of the PR and share the results here. I see the same sort of small improvements in the residual that Nick saw. FHPRTest_230610.pdf

EmyrClement · 2023-06-12T12:50:26Z

@NJManganelli I just noticed the name of the output collection is changed by this PR:

cmssw/L1Trigger/VertexFinder/python/l1tVertexProducer_cfi.py

Line 6 in 3cd6c79

    
           l1VertexCollectionName = cms.string("l1tVertices"), #Emulation postfix is appended when fastHistoEmulation is chosen as the algorithm

though this change isn't propagated to the downstream Puppi:

cmssw/L1Trigger/Phase2L1ParticleFlow/python/l1ctLayer1_cff.py

Line 22 in 3cd6c79

vtxCollection = cms.InputTag("l1tVertexFinderEmulator","l1verticesEmulation"),

unless this is modified somewhere else? Are you able to propagate this name change to the downstream consumers?

I will need to rerun my comparison of the tau efficiency with this change. I will also try to understand how I saw an improvement in the tau reconstruction efficiency given that I was likely just picking up the existing vertex from the sample.

BenjaminRS · 2023-06-12T13:02:30Z

There are a number of places this change to "l1tVerticesEmulation" would need to be updated. You can find them here: https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_13_2_X_2023-06-11-2300&_filestring=&_string=l1verticesEmulation

EmyrClement · 2023-06-14T15:55:17Z

I tested out the following recipe provided by Andrew/Benjamin:

cmsrel CMSSW_12_5_2_patch1
cd CMSSW_12_5_2_patch1/src
cmsenv
git cms-init
git cms-checkout-topic cms-l1t-offline:Phase2_prototypeSnapshot_3
git cms-rebase-topic -u epalencia:backport40563_v2
git cms-rebase-topic -u NJManganelli:l1t-phase2-fasthistoupdate-vXIII

which aims to apply this PR and a backport of the track jet PR on top of the snapshot branch we used for the Annual Review/P2UG, but I noticed this pulls in more recent changes from the IB that affects the performance of downstream objects (tau matching efficiency). I modifed the recipe to the following:

cmsrel CMSSW_12_5_2_patch1
cd CMSSW_12_5_2_patch1/src
cmsenv
git cms-init
git cms-checkout-topic cms-l1t-offline:Phase2_prototypeSnapshot_3
git cms-rebase-topic -u epalencia:backport40563_v2
git cherry-pick dda38f8b0dd0a007a50c8c6550ec8c311bef48c8

where the last line cherry-picks a commit on my own fork that is a squashed version of this PR.

The following plot shows the tau matching efficiency from the AR/P2UG snapshot (V29), the first recipe above ("Benjamin's recipe") and the second recipe above ("My recipe"). The effect of this PR on the tau efficiency is seen by comparing "V29" with "My recipe".

aloeliger · 2023-06-22T13:12:48Z

@NJManganelli I am given to understand at the software meeting yesterday that this PR is now feature complete from the track trigger perspective. At this point, because this is going to be integrated anyways, we would like to request that these commits be squashed, and a parallel PR to main CMSSW opened.

If any commits are requested in the review in CMSSW we would appreciate them finding their way back here.

triggerDoctor · 2023-06-23T13:53:37Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2023-06-23T14:10:31Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2023-06-23T14:11:07Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

NJManganelli · 2023-06-26T06:54:40Z

I reworked the commits. They are now split and squashed into 4 groups. The first and largest is basically impossible to separate further, and encompasses the split of TrackSelection/TrackVertexAssociation, propagating that workflow to GTT and SimL1, and fixing/updating the l1t naming convention in places. Group 2 is isolated to the main FastHisto update (which does depend on group 1, and the former will probably not run successfully without this). Group 3 is propagating all the workflow changes from the above updates to non-SimL1/GTT workflows. Group 4 is the L1TrackObjectNtupleMaker fixes and updates. The final diff of this 4-commit version against the previous 45-commit version reviewed/passed through trigger-doctor is completely null. This is the version I’d like to port and submit to master for review, and then proceed with back porting fixes for (to the integration branch). There are no conflicts with the base branch as of writing.

BenjaminRS · 2023-06-26T14:13:48Z

@aloeliger @epalencia - For the MET, MHT and displaced MHT collections should we go through and update the collection names in CMSSW, e.g. in the following areas:
https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_13_2_X_2023-06-22-2300&_filestring=&_string=L1TrackerEmuEtMiss
?

BenjaminRS · 2023-06-26T14:18:11Z

also I am not sure why the search did not find, for example: https://github.com/cms-l1t-offline/cmssw/blob/phase2-l1t-integration-1252patch1/L1Trigger/L1TNtuples/python/l1PhaseIITreeStep1Producer_cfi.py#L54-L57

NJManganelli · 2023-06-26T14:58:45Z

Hi everyone,

We'll do more l1t naming updates in a separate PR. This one has (without exaggeration) been rebased 20 times, and the scope creep (almost entirely my fault, up to this point) has inflated it beyond reason. This PR has a lot of testing and crosschecks, all perturbations are opportunities to introduce new bugs and delay things another few weeks. We still have 4 PRs as dependencies of this one just on my side, which also need to be tied up soon.

Best,
Nick

mcepeda · 2023-06-26T15:29:10Z

Hi Nick, there is a clear danger in merging a PR that produces collections with inconsistent naming. If the PR you are mentioning will come later gets delayed, this can break the L1 chain down the line. We have seen in the past that this kind of behaviour resulted in collections being missing in MC production (since the Event Content was asking for an object that had changed name). And for sure it introduces a misalignment between what you think has been validated and what the menu group used, since they had to manually do the changes in the collections themselves to be able to run the validation. I'm also not sure I see how fixing a few names can introduce weeks of delay.

artlbv · 2023-06-26T15:48:13Z

This file is only in the IB of L1! It’s the menu ntupler for phase 2. Therefore you cannot find it in the normal GitHub search - it does not search forks. Am 26.06.2023 um 16:18 schrieb Benjamin Radburn-Smith ***@***.***>: also I am not sure why the search did not find, for example: https://github.com/cms-l1t-offline/cmssw/blob/phase2-l1t-integration-1252patch1/L1Trigger/L1TNtuples/python/l1PhaseIITreeStep1Producer_cfi.py#L54-L57 — Reply to this email directly, view it on GitHub<#1130 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABF57TD25PPS5YGKRVCSYTLXNGK25ANCNFSM6AAAAAAYHEXF3I>. You are receiving this because you were mentioned.Message ID: ***@***.***>

kaulmer · 2023-06-26T16:50:27Z

Hi all. We clearly don't want inconsistent names. As I understand this whole naming saga, Cecile back in Sept. 2022 tried to introduce a consistent naming convention with l1t. This did not get implemented consistently everywhere back then and broke some config files for primary vertices, track MET, track jets, and track MHT. That has sat in the inconsistent state since then. While Nick is making the rest of these changes for primary vertices now, he decided to try to also fix the inconsistencies and make vertices follow the same scheme. It seems now there are at least a couple places where we missed the "l1vertices" to "l1tVertices" update. Let's clean up those l1tVertices configs now. I don't think fixing the naming convention for MET etc. has anything to do with this mammoth vertices PR.

NJManganelli · 2023-06-26T18:13:50Z

Hi Keith,

Done.

I cannot find any remaining instances with LXR in the IBs or with git grep, which still need an update.

Dear Maria,

I took that comment as an implicit request to also update the config the L1 Menu group uses, which I had avoided touching before to try to prevent a merge conflict (since they also have changes on these exact lines in that file).

Best,
Nick

artlbv · 2023-06-26T18:27:41Z

FYI here is what I used in the menu ntupler for the "v30":
https://github.com/artlbv/cmssw/blob/snap3_with_fastHisto_PR_v29ntupler_fixCherryPick/L1Trigger/L1TNtuples/python/l1PhaseIITreeStep1Producer_cfi.py#L36-L47

Introducing this change in this PR is no problem, but anyway this code is not in central CMSSW!

triggerDoctor · 2023-06-28T14:24:01Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2023-06-28T14:50:02Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2023-06-28T14:50:46Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

NJManganelli · 2023-06-28T19:45:30Z

runTheMatrix passed when run on this updated version:

runTheMatrix.py -l limited -i all --ibeos

with the following summary output:

2:46 2023; exit: 0 0 0 0 0
50 49 48 35 21 5 1 1 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 0 0 failed

I'm resolving some conflicts with the PR to master. So far, issues are with underlying discrepancies between master and the integration branch, all merge conflicts with files touched in this PR have been resolved.
ll submit the initial version as one PR, and if requested, I'll split off into 3 or 4 separate ones (L1TrackObjectNtupleMaker and the l1vertices renaming are now completely separable; others are still tied together, but might be separable with more work).

…Propagate new workflow to central L1T and GTT configurations, fix l1t broken tags l1t naming for GTTFileWriter Remove track vertex association from track selection module. Separated off track-vertex association into separate plugin. Partial update to GTT Emulation workflow, switch geometry to D88 which is the Phase2 Baseline in 12_5. add Alexx's updates to TTTrack_TrackWord.h, update to CMSSW_13 l1t tracks tag, update l1tVertexProducer default Update GTTFileReader to l1t naming. Update L1TrackJetEmulatorProducer.cc to accept TVA tracks from l1tTrackVertexAssociationProducer, unify some typedefs with other GTT modules, update l1tTrackJetsEmulation_cfi.py accordingly. Create separate copies of TS and TVA for the vertex finding, jet finding, and Missing Et. Update names to match VertexFinder configs commonly used. Add l1tVertexFinderSim and l1tVertexFinderEmu with fastHisto and fastHistoEmulation algorithms, and proper default input tags for easy process.load calls Update createFirmwareInputFiles_cfg.py for new TS, TVA, VF, JF workflow with new defaults. VertexFinderEmu to VertexFinderEmulator to sync with common usage Switch back to L1 in typedef's, update l1tTrackJets_cif.py PV input tag. l1t naming for L1Trigger/L1TTrackMatch/plugins/L1FastTrackingJetProducer.cc and L1Trigger/L1TTrackMatch/python/l1tFastTrackingJetProducer_cfi.py fixup l1t naming for GTTFileWriter TS and TVA split update, split part 1 of commit b17fb0e Multiple fixes for data flow of consumes and produces for the non-emulator GTT modules. Try to disable JF internal track selection for firmware inputs. fixup TrackJet types. fixup more fixup l1tTrackJetsEmulation_cfi.py Simplify configurations for track selection and vertex association through usage of clone. Remove debugging code from createFirmwareInputFiles_cfg.py First attempt at update to SimL1Emulator_cff.py based on new TS+VF+TVA workflow and default configurations for GTT. Fix runTheMatrix.py errors, SimL1Emulator_cff.py Update to new default on track selection and vertex association for jets, disabling external modules and keeping internal selection active

Update VertexFinder to 256 bins, delay truncation of track pt until histogramming is finished to match HDLS firmware. Update typedefs, switch vertex-finder to use selected tracks instead of converted, sync to global/EDProducer from cmssw master FastHisto update, split part 2 of commit b17fb0e Adjust VertexFinder inversion table.

…pdated JetFinding workflow to more L1T workflows Update L1TkObjectProducers_cff.py and L1TrackMET_cfg.py for TrackSelection insertion before VertexFinding Add TrackSelection in front of VertexFinding and remove redundant configuration in make_l1ctLayer1_dumpFiles_cfg.py make_l1ct_patternFiles_cfg.py l1emulator_cff.py Make input tags non-optional, ifConsumes permits non-existent tags (default) if bool is set to false

Add updated L1TrackObjectNtupleMaker code and config, removed redundant l1vertices input tags. Apply code-format updates to L1TrackObjectNtupleMaker.cc

…ranch.

NJManganelli · 2023-07-18T06:07:22Z

In addition to rebasing (resolving conflict with GTTFileWriter update), this now includes the backport (via cherry-pick) of the last commit to the PR in master as it was merged. That last commit is hopefully non-controversial, it cleans up configuration code according to Core SW recommendations and produces no word-level discrepancies in RelVals.

One thing to be noted, during rebasing, found that the l1tGTTFileWriter has miscapitalization of the "fileExtension" parameter, causing error; I'll open an issue.

triggerDoctor · 2023-07-18T07:22:15Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2023-07-18T07:25:21Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2023-07-18T07:32:05Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2023-07-18T07:32:18Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

aloeliger · 2023-07-18T12:11:30Z

At this point, because the main PR is merged, I am merging this into the phase 2 branch.

epalencia · 2023-07-25T07:47:08Z

Tagged as l1t-phase2-v72.

aloeliger added Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR labels May 24, 2023

aloeliger added the URGENT label May 24, 2023

aloeliger reviewed May 26, 2023

View reviewed changes

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from e91bf3e to 3cd6c79 Compare June 1, 2023 15:35

NJManganelli marked this pull request as ready for review June 8, 2023 18:45

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from 3cd6c79 to a35e5aa Compare June 12, 2023 19:14

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from a35e5aa to 0f738ef Compare June 26, 2023 06:46

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from 0f738ef to 0a03993 Compare June 26, 2023 17:47

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from 0a03993 to 993bf19 Compare June 28, 2023 09:07

NJManganelli mentioned this pull request Jun 29, 2023

Phase 2 forwardport FastHisto PR1130 cms-sw/cmssw#42129

Merged

Nick Manganelli added 6 commits July 18, 2023 07:15

Update L1TrackObjectNtupleMaker

290d6cc

Add updated L1TrackObjectNtupleMaker code and config, removed redundant l1vertices input tags. Apply code-format updates to L1TrackObjectNtupleMaker.cc

Harmonize l1vertices to L1Vertices

7f0c0c9

Update according to Core SW recs; backport to PR1130 in integration b…

d78e530

…ranch.

NJManganelli force-pushed the l1t-phase2-fasthistoupdate-vXIII branch from 993bf19 to d78e530 Compare July 18, 2023 05:59

aloeliger merged commit 05656d2 into cms-l1t-offline:phase2-l1t-integration-1252patch1 Jul 18, 2023

Phase 2 fastHistoEmulation updated to 256 bins, with associated Track Selection changes #1130

Phase 2 fastHistoEmulation updated to 256 bins, with associated Track Selection changes #1130

Conversation

NJManganelli commented May 19, 2023 • edited Loading

PR description:

DRAFT Updates

Updates for workflows importing the VertexProducer

Sign-off for workflows importing the VertexProducer

PR validation:

Update Validation

aloeliger commented May 24, 2023

aloeliger commented May 24, 2023

aloeliger commented May 24, 2023

gpetruc commented May 24, 2023

NJManganelli commented May 24, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpetruc commented May 26, 2023 via email

NJManganelli commented Jun 9, 2023

BenjaminRS commented Jun 10, 2023

EmyrClement commented Jun 12, 2023

BenjaminRS commented Jun 12, 2023

EmyrClement commented Jun 14, 2023

aloeliger commented Jun 22, 2023

triggerDoctor commented Jun 23, 2023

triggerDoctor commented Jun 23, 2023

triggerDoctor commented Jun 23, 2023

NJManganelli commented Jun 26, 2023

BenjaminRS commented Jun 26, 2023

BenjaminRS commented Jun 26, 2023

NJManganelli commented Jun 26, 2023

mcepeda commented Jun 26, 2023

artlbv commented Jun 26, 2023 via email

kaulmer commented Jun 26, 2023

NJManganelli commented Jun 26, 2023

artlbv commented Jun 26, 2023

triggerDoctor commented Jun 28, 2023

triggerDoctor commented Jun 28, 2023

triggerDoctor commented Jun 28, 2023

NJManganelli commented Jun 28, 2023

NJManganelli commented Jul 18, 2023

triggerDoctor commented Jul 18, 2023

triggerDoctor commented Jul 18, 2023

triggerDoctor commented Jul 18, 2023

triggerDoctor commented Jul 18, 2023

aloeliger commented Jul 18, 2023

epalencia commented Jul 25, 2023

NJManganelli commented May 19, 2023 •

edited

Loading