revert #46026 (L1-uGT emulator) #47088

missirol · 2025-01-12T17:19:21Z

PR description:

This PR updates the L1-uGT emulator by reverting #46026. Checks described in #46026 and #47030 show that #46026 introduced disagreements between the unpacked and emulated L1-uGT decisions for seeds using muons reconstructed in separate bunch crossings (specifically, the seed L1_CDC_SingleMu_3_er1p2_TOP120_DPHI2p618_3p142).

PR validation:

See #47030 (comment) and #46026 (comment).

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

CMSSW_14_2_X

Bugfix to the L1-uGT emulator, to be backported to 14_2_X (earlier cycles are not affected by the bug in question).

cmsbuild · 2025-01-12T17:19:48Z

cms-bot internal usage

cmsbuild · 2025-01-12T17:23:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47088/43267

There are other open Pull requests which might conflict with changes you have proposed:
- File L1Trigger/L1TGlobal/interface/GlobalBoard.h modified in PR(s): move per-run changes of L1-uGT emulator to beginRun (incl. loading of AXOL1TL model) #47070
- File L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.cc modified in PR(s): move per-run changes of L1-uGT emulator to beginRun (incl. loading of AXOL1TL model) #47070
- File L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.h modified in PR(s): move per-run changes of L1-uGT emulator to beginRun (incl. loading of AXOL1TL model) #47070
- File L1Trigger/L1TGlobal/src/GlobalBoard.cc modified in PR(s): move per-run changes of L1-uGT emulator to beginRun (incl. loading of AXOL1TL model) #47070

cmsbuild · 2025-01-12T17:23:32Z

A new Pull Request was created by @missirol for master.

It involves the following packages:

L1Trigger/L1TGlobal (l1)

@aloeliger, @cmsbuild, @epalencia can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @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

missirol · 2025-01-13T07:59:37Z

@cms-sw/l1-l2 @elfontan @nabrandman

Regarding the firmware-emulator mismatches for the seed L1_CDC_SingleMu_3_er1p2_TOP120_DPHI2p618_3p142 (see, for example, CMSLITDPG-1221 (comment)), my understanding is as follows, and please correct me if I'm wrong.

The mismatches are seen when using as input data the "test vectors" produced according to the recipe in Fixes mismatches between emulator and fw on CDC dimuon seed #46026 (comment).
The information encoded in the test vectors corresponds to (a) the L1T objects, and (b) the emulated decisions. The firmware decisions are produced during the test based on the L1T objects in the test vectors, and they are compared to the emulated decisions encoded in the test vectors.
The test vectors typically contain the L1T info of 3564 events from a given input file (e.g. MC or HLTPhysics data)

cmssw/L1Trigger/L1TGlobal/test/testVectorCode_data.py

Line 16 in 98dcc38

nevents = 3564 #number of events

For every event, the test vectors contain only information from BX=0, because minBxVec==maxBxVec==0

cmssw/L1Trigger/L1TGlobal/test/testVectorCode_data.py

Lines 195 to 196 in 98dcc38

    
           minBxVec       = cms.int32(0), 
        
           maxBxVec       = cms.int32(0),

cmssw/L1Trigger/L1TGlobal/plugins/GtRecordDump.cc

Lines 594 to 609 in 98dcc38

    
           // Dump Test Vectors for this bx 
        
           if (m_dumpTestVectors) { 
        
             for (int i = m_minBxVectors; i <= m_maxBxVectors; i++) { 
        
               //         if(  (i>=egammas->getFirstBX() && i<=egammas->getLastBX())&& 
        
               //	      (i>=muons->getFirstBX()   && i<=muons->getLastBX())  && 
        
               //	      (i>=taus->getFirstBX()    && i<=taus->getLastBX())   && 
        
               //	      (i>=jets->getFirstBX()    && i<=jets->getLastBX())   && 
        
               //	      (i>=etsums->getFirstBX()  && i<=etsums->getLastBX()) && 
        
               //	      (i>=uGtAlg->getFirstBX()  && i<=uGtAlg->getLastBX()) && 
        
               //	      (i>=uGtAlg->getFirstBX()  && i<=uGtAlg->getLastBX()) ) { 
        
               dumpTestVectors(i, m_testVectorFile, muons, muonShowers, egammas, taus, jets, etsums, uGtAlg, uGtExt); 
        
               //	 } else { 
        
               //	      edm::LogWarning("GtRecordDump") << "WARNING: Not enough information to dump test vectors for this bx=" << i << endl; 
        
               //	 } 
        
             } 
        
           }

If so, it seems impossible to produce the correct firmware decision for L1_CDC_SingleMu_3_er1p2_TOP120_DPHI2p618_3p142 based on these test vectors, because that seed requires a muon with certain properties in BX=-1, and the test vectors do not contain any information about that BX for any given event. The firmware decisions should always be L1_CDC_SingleMu_3_er1p2_TOP120_DPHI2p618_3p142 == false, in principle. Yet, the tests typically show that there are more events passing L1_CDC_SingleMu_3_er1p2_TOP120_DPHI2p618_3p142 in the firmware decisions, compared to the emulated decisions.

Is it possible that, in such test, the entries in the test vectors are interpreted as consecutive BXs (which is not what they are), and the firmware decisions are computed considering, for a given event, the muons of the previous event as the muons of BX=-1 of the current event ? I think that would be consistent with (a) the CDC seed accepting events in firmware, (b) the firmware and emulated decisions being different for that seed.

missirol · 2025-01-13T15:37:45Z

Is it possible that, in such test, the entries in the test vectors are interpreted as consecutive BXs (which is not what they are), and the firmware decisions are computed considering, for a given event, the muons of the previous event as the muons of BX=-1 of the current event ? I think that would be consistent with (a) the CDC seed accepting events in firmware, (b) the firmware and emulated decisions being different for that seed.

Indeed, it seems that the changes in #46026 mimic this type of (wrong) behaviour, as the muons used for BX=-1 are taken from muonVec_bxm1

cmssw/L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.cc

Line 648 in a3ba379

    
           m_uGtBrd->receiveMuonObjectData(iEvent, m_muInputToken, receiveMu, m_nrL1Mu, &muonVec_bxm2, &muonVec_bxm1);

cmssw/L1Trigger/L1TGlobal/src/GlobalBoard.cc

Lines 421 to 434 in a3ba379

    
           } else if (i == -1) { 
        
             for (std::vector<l1t::Muon>::const_iterator mu = muonVec_bxm1->begin(); mu != muonVec_bxm1->end(); ++mu) { 
        
               if (nObj < nrL1Mu) { 
        
                 (*m_candL1Mu).push_back(i, &(*mu)); 
        
               } else { 
        
                 edm::LogWarning("L1TGlobal") 
        
                     << " Too many Muons (" << nObj << ") for uGT Configuration maxMu =" << nrL1Mu; 
        
               } 
        
               LogDebug("L1TGlobal") << "Muon  Pt " << mu->hwPt() << " EtaAtVtx  " << mu->hwEtaAtVtx() << " PhiAtVtx " 
        
                                     << mu->hwPhiAtVtx() << "  Qual " << mu->hwQual() << "  Iso " << mu->hwIso(); 
        
               nObj++; 
        
             } 
        
           } else {

and the content of muonVec_bxm1 is a copy of the muons of BX=0 seen in the previous call to L1TGlobalProducer::produce (presumably, the previous event in that CMSSW stream.. ?)

cmssw/L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.cc

Lines 708 to 713 in a3ba379

    
           muonVec_bxm1.clear(); 
        
           for (std::vector<const l1t::Muon*>::const_iterator iMu = (*(m_uGtBrd->getCandL1Mu())).begin(0); 
        
                iMu != (*(m_uGtBrd->getCandL1Mu())).end(0); 
        
                ++iMu) { 
        
             muonVec_bxm1.push_back(**iMu); 
        
           }

This would explain why #46026 could reproduce the results of the firmware based on the test vectors, but in that case it is the wrong solution (if #47088 (comment) is accurate, the problem is the interpretation of the content of the test vectors, not the implementation of the emulator).

missirol · 2025-01-14T07:45:12Z

@cms-sw/l1-l2

Any feedback ? This is a bugfix, and it targets 15_0_X (which closes around Feb-4).

mmusich · 2025-01-14T07:49:15Z

type bug-fix

mmusich · 2025-01-14T07:49:21Z

@cmsbuild, please test

cmsbuild · 2025-01-14T09:44:11Z

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d1e7/43751/summary.html
COMMIT: 03091e0
CMSSW: CMSSW_15_0_X_2025-01-13-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47088/43751/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3819085
DQMHistoTests: Total failures: 67
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3818998
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 214 log files, 184 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2025-01-14T10:26:46Z

Just for the record: as expected, the tests in this PR (#47088 (comment)) show the features described in #47030 (comment) (back when this PR was part of #47030). Leaving an image below just for reference (blue points are the baseline, black line is post-PR).

aloeliger · 2025-01-14T17:30:02Z

Okay, at this point I haven't heard anything definitive back from the GT team about this. Because this seems to definitively fix an emulation bug, I am going to approve this, and then work out the issue with the GT team at a later time to allow #47030 to proceed.

aloeliger · 2025-01-14T17:30:05Z

+l1

missirol · 2025-01-14T18:11:03Z

The latest push is simply a rebase (was necessary after #47070 was merged).

cmsbuild · 2025-01-14T18:12:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47088/43286

cmsbuild · 2025-01-14T18:12:50Z

Pull request #47088 was updated. @aloeliger, @cmsbuild, @epalencia can you please check and sign again.

aloeliger · 2025-01-14T18:16:38Z

please test

cmsbuild · 2025-01-14T20:40:00Z

+1

Size: This PR adds an extra 40KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d1e7/43763/summary.html
COMMIT: 0c0e588
CMSSW: CMSSW_15_0_X_2025-01-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47088/43763/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d1e7/43763/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d1e7/43763/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 11 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3819085
DQMHistoTests: Total failures: 45
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3819020
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 214 log files, 184 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2025-01-15T12:26:18Z

#47088 (comment) also applies to the latest tests (since it's only a rebase). I double-checked that the PR is as intended post-rebase (verbatim revert).

@aloeliger , your signature was reset due to the rebase. Could you please resign ? Thanks !

aloeliger · 2025-01-15T13:08:49Z

+l1

cmsbuild · 2025-01-15T13:09:07Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @sextonkennedy, @antoniovilela, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2025-01-15T15:54:22Z

+1

missirol mentioned this pull request Jan 12, 2025

revert #46026 (L1-uGT emulator) [14_2_X] #47089

Merged

cmsbuild added this to the CMSSW_15_0_X milestone Jan 12, 2025

cmsbuild added l1-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jan 12, 2025

missirol mentioned this pull request Jan 12, 2025

Fixes mismatches between emulator and fw on CDC dimuon seed #46026

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 12, 2025

missirol mentioned this pull request Jan 12, 2025

add BX info to GlobalObjectMap (L1uGT emulation) #47030

Merged

cmsbuild added tests-started bug-fix and removed tests-pending labels Jan 14, 2025

cmsbuild added tests-approved and removed tests-started labels Jan 14, 2025

cmsbuild removed l1-pending pending-signatures labels Jan 14, 2025

cmsbuild added l1-pending pending-signatures tests-pending code-checks-pending and removed code-checks-approved labels Jan 14, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 14, 2025

cmsbuild added tests-started and removed tests-pending labels Jan 14, 2025

cmsbuild added tests-approved and removed tests-started labels Jan 14, 2025

cmsbuild added l1-approved fully-signed and removed l1-pending pending-signatures labels Jan 15, 2025

cmsbuild added orp-approved and removed orp-pending labels Jan 15, 2025

cmsbuild merged commit d968191 into cms-sw:master Jan 15, 2025
11 checks passed

missirol deleted the devel_revert46026 branch January 15, 2025 19:22

This was referenced Jan 15, 2025

Add HLT configuration for 2025 cms-data/RecoTracker-MkFit#15

Merged

Speedup of mkFit hit converters, and introduction of mkFit HLT customization for 2025 #47106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert #46026 (L1-uGT emulator) #47088

revert #46026 (L1-uGT emulator) #47088

missirol commented Jan 12, 2025

cmsbuild commented Jan 12, 2025 •

edited

Loading

cmsbuild commented Jan 12, 2025

cmsbuild commented Jan 12, 2025

missirol commented Jan 13, 2025

missirol commented Jan 13, 2025

missirol commented Jan 14, 2025

mmusich commented Jan 14, 2025

mmusich commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

missirol commented Jan 14, 2025

aloeliger commented Jan 14, 2025

aloeliger commented Jan 14, 2025

missirol commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

aloeliger commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

missirol commented Jan 15, 2025

aloeliger commented Jan 15, 2025

cmsbuild commented Jan 15, 2025

mandrenguyen commented Jan 15, 2025

revert #46026 (L1-uGT emulator) #47088

revert #46026 (L1-uGT emulator) #47088

Conversation

missirol commented Jan 12, 2025

PR description:

PR validation:

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

cmsbuild commented Jan 12, 2025 • edited Loading

cmsbuild commented Jan 12, 2025

cmsbuild commented Jan 12, 2025

missirol commented Jan 13, 2025

missirol commented Jan 13, 2025

missirol commented Jan 14, 2025

mmusich commented Jan 14, 2025

mmusich commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

Comparison Summary

missirol commented Jan 14, 2025

aloeliger commented Jan 14, 2025

aloeliger commented Jan 14, 2025

missirol commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

aloeliger commented Jan 14, 2025

cmsbuild commented Jan 14, 2025

Comparison Summary

missirol commented Jan 15, 2025

aloeliger commented Jan 15, 2025

cmsbuild commented Jan 15, 2025

mandrenguyen commented Jan 15, 2025

cmsbuild commented Jan 12, 2025 •

edited

Loading