unit test of `L1TGlobalProducer` and small technical updates to `GlobalBoard` #39832

missirol · 2022-10-23T18:17:39Z

PR description:

This PR is a follow-up of #39464. It adds a unit test for the L1TGlobalProducer plugin, and applies small technical updates to the class GlobalBoard, including the ones discussed in #39817.

Because of the new unit test, this PR is tied to a cms-data update: cms-data/L1Trigger-L1TGlobal#13.

Merely technical. No changes expected in the outputs of PR tests.

Attn: @elfontan

PR validation:

The new unit test passes in local tests.

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:

N/A

…oard

cmsbuild · 2022-10-23T18:23:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39832/32705

This PR adds an extra 28KB to repository

cmsbuild · 2022-10-23T18:24:10Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

L1Trigger/L1TGlobal (l1)

@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

missirol · 2022-10-23T18:24:30Z

test parameters:

pull_requests = input files for unit test of L1TGlobalProducer cms-data/L1Trigger-L1TGlobal#13

missirol · 2022-10-23T18:24:44Z

please test

cmsbuild · 2022-10-24T03:57:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6c6828/28453/summary.html
COMMIT: a924788
CMSSW: CMSSW_12_6_X_2022-10-23-0000/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39832/28453/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
Reco comparison had 6 failed jobs
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3378384
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3378356
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 201 log files, 48 edm output root files, 48 DQM output files
TriggerResults: no differences found

missirol

I added comments inline to guide the review.

missirol · 2022-10-24T13:01:43Z

L1Trigger/L1TGlobal/interface/GlobalBoard.h

@@ -114,7 +113,7 @@ namespace l1t {
                const int nrL1Jet);

    /// run the uGT FDL (Apply Prescales and Veto)
-    void runFDL(edm::Event& iEvent,
+    void runFDL(const edm::Event& iEvent,


Here and elsewhere, the Event arg can be made const.

missirol · 2022-10-24T13:03:24Z

L1Trigger/L1TGlobal/interface/GlobalBoard.h


-      PrescaleCounter(double prescale) : prescale_count(std::lround(prescale * m_singlestep)) {
+      PrescaleCounter(double prescale, size_t const initial_counter = 0)


I added the 2nd arg to give a handle to set the value of trigger_counter from this ctor, even though this 2nd arg is never used in the current implementation.

missirol · 2022-10-24T13:04:56Z

L1Trigger/L1TGlobal/interface/GlobalBoard.h

+    static std::vector<PrescaleCounter> prescaleCounters(std::vector<double> const& prescaleFactorsAlgoTrig);
+
+    // create prescale counters, initialising trigger_counter to a semirandom number between 0 and prescale_count - 1 inclusive
+    static std::vector<PrescaleCounter> prescaleCountersWithSemirandomInitialCounter(


I have changed the name of the method to clarify its purpose. I removed the const keyword, b/c I didn't see the advantage of returning a const vector (by value) rather than just returning a vector.

missirol · 2022-10-24T13:06:18Z

L1Trigger/L1TGlobal/src/GlobalBoard.cc

@@ -147,9 +142,8 @@ void l1t::GlobalBoard::receiveCaloObjectData(edm::Event& iEvent,
                                             const int nrL1Jet,
                                             const bool receiveEtSums) {
  if (m_verbosity) {
-    LogDebug("L1TGlobal") << "\n**** Board receiving Calo Data "
-                          //<<  "\n     from input tag " << caloInputTag << "\n"
-                          << std::endl;


Here and below, I removed the endl calls as they have no impact when passed to Log* objects.

missirol · 2022-10-24T13:10:55Z

L1Trigger/L1TGlobal/src/GlobalBoard.cc

  }

  // update and clear prescales at the beginning of the luminosity segment
-  if (m_firstEvLumiSegment || (m_currentLumi != iEvent.luminosityBlock() && m_resetPSCountersEachLumiSec)) {
+  if (m_prescaleCounterAlgoTrig.empty() or


m_firstEv and m_firstEvLumiSegment are both removed. The very first time, the block is entered because m_prescaleCounterAlgoTrig.empty() (then m_prescaleCounterAlgoTrig becomes non-empty); later, if required, the block is re-accessed if the lumisection changes.

missirol · 2022-10-24T13:11:54Z

L1Trigger/L1TGlobal/src/GlobalBoard.cc

+    auto const& prescaleCountersAlgoTrig =
+        m_semiRandomInitialPSCounters ? prescaleCountersWithSemirandomInitialCounter(prescaleFactorsAlgoTrig, iEvent)
+                                      : prescaleCounters(prescaleFactorsAlgoTrig);
+    for (int iBxInEvent = 0; iBxInEvent < totalBxInEvent; ++iBxInEvent) {


This changes the range of the loop as discussed in https://github.com/cms-sw/cmssw/pull/39464/files#r979213382. (Many) details follow.

My take is that this change makes sense, at least for the case where the relevant python modules are configured correctly ('correctly' here means how they are configured currently in production). What happens in the general case (e.g. a user making a cfg where the relevant config parameters are set arbitrarily) is less easy to judge.

The vector is m_prescaleCounterAlgoTrig, and the question is whether its size should be totalBxInEvent or totalBxInEvent + 1. The vector is only accessed once here, with index inBxInEvent = totalBxInEvent / 2 + iBxInEvent. The complication is that totalBxInEvent and iBxInEvent come from independent parameters (also, note that m_uGtAlgBlk depends on iBxInEvent, due to the preceding call to runGTL in L1TGlobalProducer).

totalBxInEvent comes from (StableParametersTrivialProducer).TotalBxInEvent (where (StableParametersTrivialProducer) means the instance of the ESProducer StableParametersTrivialProducer in the cfg file) [1], while iBxInEvent runs from minEmulBxInEvent to maxEmulBxInEvent (both included), which is related to (L1TGlobalProducer).EmulateBxInEvent [2].

If (StableParametersTrivialProducer).TotalBxInEvent (default: 5) is odd and larger or equal to (L1TGlobalProducer).EmulateBxInEvent (default: 1), then the index totalBxInEvent / 2 + iBxInEvent goes at most up to totalBxInEvent / 2 + ((totalBxInEvent+1) / 2 - 1) = totalBxInEvent - 1, meaning that the correct choice is size = totalBxInEvent (not totalBxInEvent + 1).

What happens in the (unlikely?) case (StableParametersTrivialProducer).TotalBxInEvent < (L1TGlobalProducer).EmulateBxInEvent is much less clear to me. The unit test on MC did not crash, and returned the same results, even in this case (inBxInEvent can go out-of-range in this case, but the plugin didn't not evaluate the vector m_prescaleCounterAlgoTrig because no algo was accepting events for those BXs).

[1] totalBxInEvent comes from

cmssw/L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.cc

Line 269 in a993c0f

m_totalBxInEvent = data->totalBxInEvent();

and I think this number corresponds to the config parameter (StableParametersTrivialProducer).TotalBxInEvent (where (StableParametersTrivialProducer) means the instance of the ESProducer StableParametersTrivialProducer in the cfg file). IIuc, in stage-2 L1T this is 5 (meaning the BXs are "-2,-1,0,1,2"), but in principle it could be set to anything.

[2] iBxInEvent goes from minEmulBxInEvent to maxEmulBxInEvent (both included):

minEmulBxInEvent and maxEmulBxInEvent are set based on m_emulateBxInEvent.

The value of m_emulateBxInEvent can be set independently of TotalBxInEvent.

m_emulateBxInEvent is forced to be odd, unless it is negative and in that case m_emulateBxInEvent is set to totalBxInEvent (where totalBxInEvent can in principle be anything).

It seems like the idea is that m_emulateBxInEvent is odd, and then min and max are the first and last BX (e.g. if m_emulateBxInEvent=7, minEmulBxInEvent=-3 and maxEmulBxInEvent=3); but again, there is an edge case where m_emulateBxInEvent could be even (e.g. if (L1TGlobalProducer).EmulateBxInEvent = -1 and (StableParametersTrivialProducer).TotalBxInEvent is even), and the L1T producer does not seem to guard against that.

In reality, we use m_emulateBxInEvent == 1 and minEmulBxInEvent == maxEmulBxInEvent == 0 (meaning, only the "central" BX is considered).

cmssw/L1Trigger/L1TGlobal/plugins/L1TGlobalProducer.cc

Lines 317 to 318 in a993c0f

int minEmulBxInEvent = (m_emulateBxInEvent + 1) / 2 - m_emulateBxInEvent;

int maxEmulBxInEvent = (m_emulateBxInEvent + 1) / 2 - 1;

missirol · 2022-10-24T13:12:53Z

L1Trigger/L1TGlobal/src/GlobalBoard.cc


-  LogTrace("L1TGlobal") << std::endl;
+// initialize prescale counters to zero
+std::vector<l1t::GlobalBoard::PrescaleCounter> l1t::GlobalBoard::prescaleCounters(


This is not a new method; I just moved up an old one, and renamed it.

cecilecaillol · 2022-10-25T07:01:12Z

+l1

cmsbuild · 2022-10-25T07:01:31Z

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

rappoccio · 2022-10-25T12:12:03Z

@cecilecaillol can you also sign the cms-data PR? cms-data/L1Trigger-L1TGlobal#13

Thanks!
Sal

rappoccio · 2022-10-25T15:23:02Z

+1

Discussed at ORP, can go into pre4.

unit test of L1TGlobalProducer and small technical updates to GlobalB…

a924788

…oard

missirol mentioned this pull request Oct 23, 2022

input files for unit test of L1TGlobalProducer cms-data/L1Trigger-L1TGlobal#13

Merged

cmsbuild added this to the CMSSW_12_6_X milestone Oct 23, 2022

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

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

cmsbuild added the requires-external label Oct 23, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 23, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 24, 2022

missirol commented Oct 24, 2022

View reviewed changes

missirol mentioned this pull request Oct 24, 2022

Review and optimize Fractional Prescale GlobalBoard loops #39817

Closed

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Oct 25, 2022

cmsbuild added orp-approved and removed orp-pending labels Oct 25, 2022

cmsbuild merged commit 8b4a7dc into cms-sw:master Oct 25, 2022

missirol deleted the devel_l1tGlobalProducerTest branch October 25, 2022 19:22

missirol mentioned this pull request Oct 25, 2022

update of L1TGlobalProducer to handle fractional prescales [12_5_X] #39851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unit test of `L1TGlobalProducer` and small technical updates to `GlobalBoard` #39832

unit test of `L1TGlobalProducer` and small technical updates to `GlobalBoard` #39832

missirol commented Oct 23, 2022 •

edited

cmsbuild commented Oct 23, 2022

cmsbuild commented Oct 23, 2022

missirol commented Oct 23, 2022

missirol commented Oct 23, 2022

cmsbuild commented Oct 24, 2022

missirol left a comment

missirol Oct 24, 2022

missirol Oct 24, 2022

missirol Oct 24, 2022

missirol Oct 24, 2022

missirol Oct 24, 2022

missirol Oct 24, 2022

missirol Oct 24, 2022

cecilecaillol commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

rappoccio commented Oct 25, 2022

rappoccio commented Oct 25, 2022


		PrescaleCounter(double prescale) : prescale_count(std::lround(prescale * m_singlestep)) {
		PrescaleCounter(double prescale, size_t const initial_counter = 0)

	int minEmulBxInEvent = (m_emulateBxInEvent + 1) / 2 - m_emulateBxInEvent;
	int maxEmulBxInEvent = (m_emulateBxInEvent + 1) / 2 - 1;

unit test of L1TGlobalProducer and small technical updates to GlobalBoard #39832

unit test of L1TGlobalProducer and small technical updates to GlobalBoard #39832

Conversation

missirol commented Oct 23, 2022 • edited

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 Oct 23, 2022

cmsbuild commented Oct 23, 2022

missirol commented Oct 23, 2022

missirol commented Oct 23, 2022

cmsbuild commented Oct 24, 2022

Comparison Summary

missirol left a comment

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

missirol Oct 24, 2022

Choose a reason for hiding this comment

cecilecaillol commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

rappoccio commented Oct 25, 2022

rappoccio commented Oct 25, 2022

unit test of `L1TGlobalProducer` and small technical updates to `GlobalBoard` #39832

unit test of `L1TGlobalProducer` and small technical updates to `GlobalBoard` #39832

missirol commented Oct 23, 2022 •

edited