Add GCTDigiClusterLink for representing link between a GCT card to CTL1 by pallabidas · Pull Request #149 · p2l1pfp/cmssw

pallabidas · 2026-01-21T13:35:57Z

PR description:

Attempt to add a vector GCTDigiClusterLink of std::variant<std::monostate, l1tp2::GCTEmDigiCluster, l1tp2::GCTHadDigiCluster> GCTDigiCluster for representing one link per GCT card. Following geometry in https://www.dropbox.com/scl/fi/1m47c74c19gt1vks9etc5/2025-01-17-Updated-Documentation-Barrel-GCT-Clusters-to-Correlator-Layer-1.pdf?rlkey=rottdsivjj8t0fl08l338r6bk&e=1&dl=0 but includes newer digi definitions. Checked that the GCTDigiCluster collection can be accessed in an analyzer.

jmitrevs · 2026-01-21T18:16:10Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h

+  class DigitizedCaloToCorrelatorTMI18 {
+  private:
+    // Data
+    ap_uint<64> Card0Data[162] ;


The Card*Data is redundant since the exact same information is in Card*Link. Basically Card0Link[5].data() == Card0Data[5]. My preference would be to remove Card*Data since one runs into possibilities of diverging values without really giving you anything extra.

For now we keep it for cross check by Sascha

jmitrevs · 2026-01-21T18:18:26Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h

+    const ap_uint<64>*  dataCard1() const { return Card1Data; }
+    const ap_uint<64>*  dataCard2() const { return Card2Data; }
+    const GCTDigiClusterLink& linkCard0() const { return Card0Link; }
+    const GCTDigiClusterLink& linkCard1() const { return Card1Link; }


I think having 0, 1, or 2 being parameters passed to one function is easier than having distinct member function names. This can also be solved with the comment below, where the collection is not of size 1 but of size 3.

jmitrevs · 2026-01-21T18:23:38Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h

+
+  // Collection typedef
+  // this represents both the EM and PF clusters from a single GCT card in one link
+  typedef std::vector<l1tp2::DigitizedCaloToCorrelatorTMI18> DigitizedCaloToCorrelatorCollectionTMI18;


Currently this is always a vector of size 1, with the internal object having 3 GCTDigiClusterLink objects. I have a slight preference to having DigitizedCaloToCorrelatorCollectionTMI18 be a vector of size 3, with each entry being one link. If we in particular get rid of the duplicate information, I would propose having a the following definition:

typedef std::vector<l1tp2::GCTDigiClusterLink> DigitizedCaloToCorrelatorCollectionTMI18;

and not even defining a DigitizedCaloToCorrelatorTMI18 class.

this is now a vector of links

It is a vector of structures {CardData[i]=CardLink[i].data(), CardLink}. My plan for the CL1 regionzier is to ignore the duplicate CardData that take all the information from CardLink.

ok, as discussed before, we will remove CardLink.data in future iteration

Just to clarify, in the future you'll remove CardData; the data will always be available in CardLink, right? Otherwise the accessors would stop working.

yes, we will remove CardData only and keep using CardLink. sorry for the confusion!

jmitrevs · 2026-01-21T18:36:37Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h

+#include <vector>
+#include "DataFormats/L1TCalorimeterPhase2/interface/GCTEmDigiCluster.h"
+#include "DataFormats/L1TCalorimeterPhase2/interface/GCTHadDigiCluster.h"
+#include "DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h"


Is this include needed?

This is now fixed.

cerminar · 2026-01-22T11:22:32Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h

+    ap_uint<7> digitizeEta(unsigned int iEtaCr) { return (ap_uint<7>)iEtaCr; }

-    ap_uint<7> digitizeIPhiCr(unsigned int iPhiCr) { return (ap_uint<7>)iPhiCr; }
+    ap_uint<7> digitizePhi(unsigned int iPhiCr) { return (ap_uint<7>)iPhiCr; }


Are these actually used/needed? I see that there are other functions with the same name for the digitizatin of the actual phi and eta (not the indexes)....

Copilot

Pull request overview

This pull request adds a new GCTDigiClusterLink data structure to represent links between GCT cards and the Correlator Layer 1 (CTL1). The implementation uses std::variant to handle both electromagnetic and hadronic cluster types, with updates to digitization and geometry definitions across multiple files.

Changes:

Introduces new GCTDigiClusterLink typedef using std::variant<std::monostate, GCTEmDigiCluster, GCTHadDigiCluster> to represent cluster data
Updates region ordering from GCT1/2/3 SLR1/3 to SLR3/1 to match revised geometry documentation
Modifies cluster digitization to use new bit packing schemes with placeholder values for incomplete fields

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 29 comments.

Show a summary per file

File	Description
L1Trigger/L1CaloTrigger/python/l1tPhase2CaloToCorrelatorTMI18_cfi.py	New configuration file defining input tags for the TMI18 producer
L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloToCorrelatorTMI18.cc	New producer implementing cluster-to-correlator mapping with goto-based control flow
L1Trigger/L1CaloTrigger/plugins/Phase2L1TCaloBarrelToCorrelator.cc	Updates SLR region ordering and implements new phi/eta digitization with placeholder values
L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloPFClusterEmulator.cc	Fixes phi wrapping and eta offset calculation bugs
L1Trigger/L1CaloTrigger/interface/Phase2L1CaloToCorrelatorTMI18.h	Header file with unnecessary includes and commented code
L1Trigger/L1CaloTrigger/interface/Phase2L1CaloPFClusterEmulator.h	Updates peak position calculation and cluster position handling
L1Trigger/L1CaloTrigger/interface/Phase2L1CaloEGammaUtils.h	Updates tower counts, cluster limits, and digitization logic with placeholder values
DataFormats/L1TCalorimeterPhase2/src/classes_def.xml	Adds dictionary definitions for new digitized jet and TMI18 classes
DataFormats/L1TCalorimeterPhase2/src/classes.h	Includes new header files for dictionary generation
DataFormats/L1TCalorimeterPhase2/interface/GCTHadDigiCluster.h	Redesigns constructor and getters with new bit packing scheme
DataFormats/L1TCalorimeterPhase2/interface/GCTEmDigiCluster.h	Redesigns constructor and getters with new bit packing scheme
DataFormats/L1TCalorimeterPhase2/interface/DigitizedL1CaloJet.h	New class for digitized jet representation
DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h	Updates bit packing and adds realPhi/cardNumber calculation logic
DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h	New header defining the TMI18 data structure with GCTDigiClusterLink

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T11:37:48Z

L1Trigger/L1CaloTrigger/interface/Phase2L1CaloEGammaUtils.h

+          et,  // technically we are just multiplying and then dividing again by the LSB
+          abseta,
+          phivscenter,
+          ap_uint<6>(0x3F) ,
+          ap_uint<6>(0x3F) ,
+          ap_uint<6>(0x3F) ,
+          ap_uint<3>(0x7) ,
+          ap_uint<5>(timing),
+          ap_uint<2>(brems),
+          ap_uint<10>(spare) ,
+          nGCTCard, true ) ;


Placeholder values (0x3F, 0x7, etc.) are being used for hoe, iso, shape, and wp parameters. Document what these maximum values represent and whether they should be replaced with actual computed values in future updates.

Suggested change

et, // technically we are just multiplying and then dividing again by the LSB

abseta,

phivscenter,

ap_uint<6>(0x3F) ,

ap_uint<6>(0x3F) ,

ap_uint<6>(0x3F) ,

ap_uint<3>(0x7) ,

ap_uint<5>(timing),

ap_uint<2>(brems),

ap_uint<10>(spare) ,

nGCTCard, true ) ;

et, // cluster ET (technically we are just multiplying and then dividing again by the LSB)

abseta, // |eta| encoded for the correlator

phivscenter,

ap_uint<6>(0x3F), // hoe: placeholder max (6 bits all 1); currently "not computed"/no H/E information

ap_uint<6>(0x3F), // iso: placeholder max (6 bits all 1); currently "not computed"/no isolation encoding

ap_uint<6>(0x3F), // shape: placeholder max (6 bits all 1); currently "not computed"/no shower-shape encoding

ap_uint<3>(0x7), // wp: placeholder max (3 bits all 1); currently "not computed"/no WP encoding

ap_uint<5>(timing),

ap_uint<2>(brems),

ap_uint<10>(spare),

nGCTCard,

true // valid flag

) ;

Copilot · 2026-01-22T11:37:49Z

L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloToCorrelatorTMI18.cc

+	if (isSLR1 && isNegativeEta) goto slr1negp;
+	if (isSLR1 && !isNegativeEta) goto slr1posp;
+	if (isSLR3 && isNegativeEta) goto slr3negp;
+	if (isSLR3 && !isNegativeEta) goto slr3posp;
+
+slr3posp:  ;
+
+        if(iGCT == 0 && cntr03pos < 24){
+          dataToCL1Card0[17+cntr03pos] = mydata;
+	  clusterCollCard0[17+cntr03pos] = cluster;
+          cntr03pos++;  
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr13pos < 24){
+          dataToCL1Card1[17+cntr13pos] = mydata;
+	  clusterCollCard1[17+cntr13pos] = cluster;
+          cntr13pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr23pos < 24){
+          dataToCL1Card2[17+cntr23pos] = mydata;
+	  clusterCollCard2[17+cntr23pos] = cluster;
+          cntr23pos++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr3negp:  ;
+
+        if(iGCT == 0 && cntr03neg < 24){
+          dataToCL1Card0[57+cntr03neg] = mydata;
+	  clusterCollCard0[57+cntr03neg] = cluster;
+          cntr03neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr13neg < 24){
+          dataToCL1Card1[57+cntr13neg] = mydata;
+	  clusterCollCard1[57+cntr13neg] = cluster;
+          cntr13neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr23neg < 24){
+          dataToCL1Card2[57+cntr23neg] = mydata;
+	  clusterCollCard2[57+cntr23neg] = cluster;
+          cntr23neg++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr1posp:  ;
+
+        if(iGCT == 0 && cntr01pos < 24){
+          dataToCL1Card0[81+17+cntr01pos] = mydata;
+	  clusterCollCard0[81+17+cntr01pos] = cluster;
+          cntr01pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr11pos < 24){
+          dataToCL1Card1[81+17+cntr11pos] = mydata;
+	  clusterCollCard1[81+17+cntr11pos] = cluster;
+          cntr11pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr21pos < 24){
+          dataToCL1Card2[81+17+cntr21pos] = mydata;
+	  clusterCollCard2[81+17+cntr21pos] = cluster;
+          cntr21pos++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr1negp:  ;
+
+        if(iGCT == 0 && cntr01neg < 24){
+          dataToCL1Card0[81+57+cntr01neg] = mydata;
+	  clusterCollCard0[81+57+cntr01neg] = cluster;
+          cntr01neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr11neg < 24){
+          dataToCL1Card1[81+57+cntr11neg] = mydata;
+	  clusterCollCard1[81+57+cntr11neg] = cluster;
+          cntr11neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr21neg < 24){
+          dataToCL1Card2[81+57+cntr21neg] = mydata;
+	  clusterCollCard2[81+57+cntr21neg] = cluster;
+          cntr21neg++;
+          goto fillendp; 
+        } 
+
+fillendp:  ;


The extensive use of goto statements makes the control flow difficult to follow and maintain. Consider refactoring to use structured control flow with functions or switch statements. Modern C++ practice discourages goto usage except in very specific low-level scenarios.

Copilot · 2026-01-22T11:37:49Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h

+      if (phiInDegrees > 160 && phiInDegrees < 280) cardnumber = 0;
+      if ((phiInDegrees > 280 && phiInDegrees < 360) || phiInDegrees < 40) cardnumber = 1;
+      if (phiInDegrees > 40 && phiInDegrees < 160) cardnumber = 2;
+      if (pt() == 96) std::cout<<realPhi()<<"\t"<<phiInDegrees<<"\t"<<cardnumber<<std::endl;


Debug print statement should be removed or converted to proper logging. If needed for debugging, use edm::LogVerbatim or conditional compilation guards.

Suggested change

if (pt() == 96) std::cout<<realPhi()<<"\t"<<phiInDegrees<<"\t"<<cardnumber<<std::endl;

#ifdef EDM_ML_DEBUG

if (pt() == 96)

std::cout << realPhi() << "\t" << phiInDegrees << "\t" << cardnumber << std::endl;

#endif

Copilot · 2026-01-22T11:37:50Z

L1Trigger/L1CaloTrigger/plugins/Phase2L1TCaloBarrelToCorrelator.cc

+	float absphi = pfIn.clusterPhi()-0.525 ;
+	float phivscenter = 0;
+	ap_int<7> pf_phi = 0;
+	if(absphi>=-1.575 && absphi < -0.525) { phivscenter = absphi + 1.05 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ; }  // first 2 bits encode slr 1 or 3
+	if(absphi>=-0.525 && absphi < 0.525) { phivscenter = absphi ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ;}
+	if(absphi>=0.525 && absphi < 1.575) { phivscenter = absphi - 1.05 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ;} // bits 3 and 4 encode the cards 0 1 3 as 0  8  16
+	if(absphi>=1.575 && absphi < 2.625) { phivscenter = absphi - 2.1 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ;}
+	if(absphi>=2.625 ) { phivscenter = absphi - 3.15 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ;  }
+	if(absphi<-2.625) { phivscenter = absphi + 3.15 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ;  }
+	if(absphi>=-2.625 && absphi < -1.575) { phivscenter = absphi + 2.1 ; pf_phi = (ap_int<7>)(phivscenter/0.525*30) ; }


The variable phivscenter is computed but may be left uninitialized if absphi doesn't match any of the conditional ranges. This could lead to undefined behavior. Ensure all possible values of absphi are handled, or initialize phivscenter and pf_phi to default values.

Copilot · 2026-01-22T11:37:50Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h

+      clusterData = ((ap_uint<64>)pt) | (((ap_uint<64>)eta) << 12) | (((ap_uint<64>)(phi & 0x7F)) << 19) |
+                    (((ap_uint<64>)hoe) << 26) | (((ap_uint<64>)iso) << 32) |
+                    (((ap_uint<64>)shape) << 38) | (((ap_uint<64>)wp) << 44) | (((ap_uint<64>)timing) << 47) |
+                    (((ap_uint<64>)brems) << 52) | (((ap_uint<64>)spare) << 54);


Masking with 0x7F on a signed ap_int<7> may not preserve the sign correctly. The mask will extract 7 bits, but the sign interpretation depends on the context. Verify that this masking operation correctly handles negative phi values.

Suggested change

clusterData = ((ap_uint<64>)pt) | (((ap_uint<64>)eta) << 12) | (((ap_uint<64>)(phi & 0x7F)) << 19) |

(((ap_uint<64>)hoe) << 26) | (((ap_uint<64>)iso) << 32) |

(((ap_uint<64>)shape) << 38) | (((ap_uint<64>)wp) << 44) | (((ap_uint<64>)timing) << 47) |

(((ap_uint<64>)brems) << 52) | (((ap_uint<64>)spare) << 54);

clusterData = ((ap_uint<64>)pt) | (((ap_uint<64>)eta) << 12) |

(((ap_uint<64>)((ap_uint<7>)phi)) << 19) | (((ap_uint<64>)hoe) << 26) |

(((ap_uint<64>)iso) << 32) | (((ap_uint<64>)shape) << 38) | (((ap_uint<64>)wp) << 44) |

(((ap_uint<64>)timing) << 47) | (((ap_uint<64>)brems) << 52) | (((ap_uint<64>)spare) << 54);

It has been verified that masking preserves the sign

Copilot · 2026-01-22T11:37:55Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedCaloToCorrelatorTMI18.h

+	    for (int i = 0; i < 162; i++) {
+		    Card0Data[i]=0;
+		    Card1Data[i]=0;
+		    Card2Data[i]=0;
+	    }
+    }
+    DigitizedCaloToCorrelatorTMI18(ap_uint<64> data0[162], ap_uint<64> data1[162], ap_uint<64> data2[162], GCTDigiClusterLink link0, GCTDigiClusterLink link1, GCTDigiClusterLink link2) { 
+	    for (int i = 0; i < 162; i++) {
+		    Card0Data[i]=data0[i];
+		    Card1Data[i]=data1[i];
+		    Card2Data[i]=data2[i];
+	    }
+	    Card0Link = link0;
+	    Card1Link = link1;
+	    Card2Link = link2;


Inconsistent indentation detected with tabs being used instead of spaces. This should be standardized according to the project's coding style.

Suggested change

for (int i = 0; i < 162; i++) {

Card0Data[i]=0;

Card1Data[i]=0;

Card2Data[i]=0;

}

}

DigitizedCaloToCorrelatorTMI18(ap_uint<64> data0[162], ap_uint<64> data1[162], ap_uint<64> data2[162], GCTDigiClusterLink link0, GCTDigiClusterLink link1, GCTDigiClusterLink link2) {

for (int i = 0; i < 162; i++) {

Card0Data[i]=data0[i];

Card1Data[i]=data1[i];

Card2Data[i]=data2[i];

}

Card0Link = link0;

Card1Link = link1;

Card2Link = link2;

for (int i = 0; i < 162; i++) {

Card0Data[i]=0;

Card1Data[i]=0;

Card2Data[i]=0;

}

}

DigitizedCaloToCorrelatorTMI18(ap_uint<64> data0[162], ap_uint<64> data1[162], ap_uint<64> data2[162], GCTDigiClusterLink link0, GCTDigiClusterLink link1, GCTDigiClusterLink link2) {

for (int i = 0; i < 162; i++) {

Card0Data[i]=data0[i];

Card1Data[i]=data1[i];

Card2Data[i]=data2[i];

}

Card0Link = link0;

Card1Link = link1;

Card2Link = link2;

Copilot · 2026-01-22T11:37:56Z

L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloToCorrelatorTMI18.cc

+#include <array>
+#include <cmath>
+#include <typeinfo>
+// #include <cstdint>


Commented-out include statement should be removed if it's not needed.

Suggested change

// #include <cstdint>

Copilot · 2026-01-22T11:37:56Z

DataFormats/L1TCalorimeterPhase2/interface/DigitizedClusterCorrelator.h

+      int crPhi = phi() % 5;
+      if (phi() < 0) crPhi = (30 + phi()) % 5;


The modulo operation on a potentially negative phi value may not behave as expected. In C++, the result of modulo with negative numbers can be negative. Consider using a proper wrapping function or ensuring phi is non-negative before the modulo operation.

Suggested change

int crPhi = phi() % 5;

if (phi() < 0) crPhi = (30 + phi()) % 5;

int phiVal = phi();

int crPhi = ((phiVal % 5) + 5) % 5;

Copilot · 2026-01-22T11:37:56Z

DataFormats/L1TCalorimeterPhase2/interface/GCTHadDigiCluster.h

+    //ap_uint<12> pt() const { return data().range(11, 0); }
+    ap_uint<12> pt() const { return (clusterData & 0xFFF); }
    float ptFloat() const { return pt() * ptLSB(); }

    // crystal eta (unsigned 7 bits)
-    int eta() const { return (ap_uint<7>)data().range(18, 12); }
+    //int eta() const { return (ap_uint<7>)data().range(18, 12); }
+    ap_uint<7> eta() const { return ((clusterData >> 12) & 0x7F); }  // (eight 1's) 0b11111111 = 0xFF

    // crystal phi (signed 7 bits)
-    int phi() const { return (ap_int<7>)data().range(25, 19); }
+    //int phi() const { return (ap_int<7>)data().range(25, 19); }
+    ap_int<7> phi() const { return ((clusterData >> 19) & 0x7F); }  // (seven 1's) 0b1111111 = 0x7F
+
+    // timing: not saved in the current emulator
+    ap_uint<12> ecal() const { return ((clusterData >> 26) & 0xFFF); }
+    ap_uint<6> fb() const { return ((clusterData >> 38) & 0x3F); }
+
+    // brems: not saved in the current emulator
+    ap_uint<10> spare() const { return ((clusterData >> 44) & 0xFFFFF); }

    // HoE value
-    ap_uint<4> hoe() const { return data().range(30, 26); }
+    //ap_uint<4> hoe() const { return data().range(30, 26); }


Commented-out code for getters should be removed. Keep the codebase clean by removing unused implementations.

Copilot · 2026-01-22T11:37:57Z

L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloToCorrelatorTMI18.cc

+        if(iGCT == 0 && cntr03pos < 24){
+          dataToCL1Card0[17+cntr03pos] = mydata;
+	  clusterCollCard0[17+cntr03pos] = cluster;
+          cntr03pos++;  
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr13pos < 24){
+          dataToCL1Card1[17+cntr13pos] = mydata;
+	  clusterCollCard1[17+cntr13pos] = cluster;
+          cntr13pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr23pos < 24){
+          dataToCL1Card2[17+cntr23pos] = mydata;
+	  clusterCollCard2[17+cntr23pos] = cluster;
+          cntr23pos++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr3negp:  ;
+
+        if(iGCT == 0 && cntr03neg < 24){
+          dataToCL1Card0[57+cntr03neg] = mydata;
+	  clusterCollCard0[57+cntr03neg] = cluster;
+          cntr03neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr13neg < 24){
+          dataToCL1Card1[57+cntr13neg] = mydata;
+	  clusterCollCard1[57+cntr13neg] = cluster;
+          cntr13neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr23neg < 24){
+          dataToCL1Card2[57+cntr23neg] = mydata;
+	  clusterCollCard2[57+cntr23neg] = cluster;
+          cntr23neg++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr1posp:  ;
+
+        if(iGCT == 0 && cntr01pos < 24){
+          dataToCL1Card0[81+17+cntr01pos] = mydata;
+	  clusterCollCard0[81+17+cntr01pos] = cluster;
+          cntr01pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr11pos < 24){
+          dataToCL1Card1[81+17+cntr11pos] = mydata;
+	  clusterCollCard1[81+17+cntr11pos] = cluster;
+          cntr11pos++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr21pos < 24){
+          dataToCL1Card2[81+17+cntr21pos] = mydata;
+	  clusterCollCard2[81+17+cntr21pos] = cluster;
+          cntr21pos++;
+          goto fillendp;
+        } 
+        goto fillendp;
+
+slr1negp:  ;
+
+        if(iGCT == 0 && cntr01neg < 24){
+          dataToCL1Card0[81+57+cntr01neg] = mydata;
+	  clusterCollCard0[81+57+cntr01neg] = cluster;
+          cntr01neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 1 && cntr11neg < 24){
+          dataToCL1Card1[81+57+cntr11neg] = mydata;
+	  clusterCollCard1[81+57+cntr11neg] = cluster;
+          cntr11neg++;
+          goto fillendp;
+        } 
+        if(iGCT == 2 && cntr21neg < 24){
+          dataToCL1Card2[81+57+cntr21neg] = mydata;
+	  clusterCollCard2[81+57+cntr21neg] = cluster;
+          cntr21neg++;
+          goto fillendp; 
+        } 
+
+fillendp:  ;
+      }
+    }
+  }
+
+  cntr03pos = 0 ;
+  cntr03neg = 0 ;
+  cntr01pos = 0 ;
+  cntr01neg = 0 ;
+
+  cntr13pos = 0 ;
+  cntr13neg = 0 ;
+  cntr11pos = 0 ;
+  cntr11neg = 0 ;
+
+  cntr23pos = 0 ;
+  cntr23neg = 0 ;
+  cntr21pos = 0 ;
+  cntr21neg = 0 ;
+
+  edm::Handle<l1tp2::GCTEmDigiClusterCollection> gctEmDigiClusters;
+  if(evt.getByToken(gctEmDigiClustersSrc_, gctEmDigiClusters)){
+    for (int iLink = 0; iLink < 12; iLink++) {
+      // in order:
+      // GCT1 SLR3 +ve, GCT1 SLR3 -ve, GCT1 SLR1 +ve, GCT1 SLR1 -ve
+      // GCT2 SLR3 +ve, GCT2 SLR3 -ve, GCT2 SLR1 +ve, GCT2 SLR1 -ve
+      // GCT3 SLR3 +ve, GCT3 SLR3 -ve, GCT3 SLR1 +ve, GCT3 SLR1 -ve
+      int iGCT = (iLink/4);
+      // 4 RCT regions per GCT
+      int iRCT = iLink - iGCT*4;
+      // Eta: positive or negative depends on the link. If iLink is even, it is in positive eta
+      bool isNegativeEta = (iRCT % 2 == 1);
+      // SLR alternates every two links
+      bool isSLR3 = ((iLink % 4) < 2);
+      bool isSLR1 = !isSLR3;
+      for (const auto & cluster : (*gctEmDigiClusters).at(iLink)) {
+        mydata = cluster.data() ;
+	if (isSLR1 && isNegativeEta) goto slr1neg;
+	if (isSLR1 && !isNegativeEta) goto slr1pos;
+	if (isSLR3 && isNegativeEta) goto slr3neg;
+	if (isSLR3 && !isNegativeEta) goto slr3pos;
+
+slr3pos:  ;
+
+        if(iGCT == 0 && cntr03pos < 16){
+          dataToCL1Card0[1+cntr03pos] = mydata;
+	  clusterCollCard0[1+cntr03pos] = cluster;
+          cntr03pos++;
+          goto fillend;
+        } 
+        if(iGCT == 1 && cntr13pos < 16){
+          dataToCL1Card1[1+cntr13pos] = mydata;
+	  clusterCollCard1[1+cntr13pos] = cluster;
+          cntr13pos++;
+          goto fillend;
+        } 
+        if(iGCT == 2 && cntr23pos < 16){
+          dataToCL1Card2[1+cntr23pos] = mydata;
+	  clusterCollCard2[1+cntr23pos] = cluster;
+          cntr23pos++;
+          goto fillend;
+        }
+	goto fillend;
+
+slr3neg:  ;
+
+        if(iGCT == 0 && cntr03neg < 16){
+          dataToCL1Card0[41+cntr03neg] = mydata;
+	  clusterCollCard0[41+cntr03neg] = cluster;
+          cntr03neg++;
+          goto fillend;
+        } 
+        if(iGCT == 1 && cntr13neg < 16){
+          dataToCL1Card1[41+cntr13neg] = mydata;
+	  clusterCollCard1[41+cntr13neg] = cluster;
+          cntr13neg++;
+          goto fillend;
+        } 
+        if(iGCT == 2 && cntr23neg < 16){
+          dataToCL1Card2[41+cntr23neg] = mydata;
+	  clusterCollCard2[41+cntr23neg] = cluster;
+          cntr23neg++;
+          goto fillend;
+        }
+	goto fillend;
+
+slr1pos:  ;
+
+        if(iGCT == 0 && cntr01pos < 16){
+          dataToCL1Card0[81+1+cntr01pos] = mydata;
+	  clusterCollCard0[81+1+cntr01pos] = cluster;
+          cntr01pos++;
+          goto fillend;
+        } 
+        if(iGCT == 1 && cntr11pos < 16){
+          dataToCL1Card1[81+1+cntr11pos] = mydata;
+	  clusterCollCard1[81+1+cntr11pos] = cluster;
+          cntr11pos++;
+          goto fillend;
+        } 
+        if(iGCT == 2 && cntr21pos < 16){
+          dataToCL1Card2[81+1+cntr21pos] = mydata;
+	  clusterCollCard2[81+1+cntr21pos] = cluster;
+          cntr21pos++;
+          goto fillend;
+        }
+	goto fillend;
+
+slr1neg:  ;
+
+        if(iGCT == 0 && cntr01neg < 16){
+          dataToCL1Card0[81+41+cntr01neg] = mydata;
+	  clusterCollCard0[81+41+cntr01neg] = cluster;
+          cntr01neg++;
+          goto fillend;
+        } 
+        if(iGCT == 1 && cntr11neg < 16){
+          dataToCL1Card1[81+41+cntr11neg] = mydata;
+	  clusterCollCard1[81+41+cntr11neg] = cluster;
+          cntr11neg++ ;
+          goto fillend ;
+        } 
+        if(iGCT == 2 && cntr21neg < 16){
+          dataToCL1Card2[81+41+cntr21neg] = mydata;
+	  clusterCollCard2[81+41+cntr21neg] = cluster;
+          cntr21neg++ ;
+          goto fillend ;
+        }


Potential array out-of-bounds access if cntr values exceed 23 (for EM clusters) or 15 (for Had clusters). While there are checks like "cntr03pos < 24", there's no protection if more clusters arrive than expected. Consider adding bounds checking or logging a warning if the limit is exceeded.

…yer-1 PF algo

pallabidas added 2 commits January 21, 2026 14:25

testing new EM and PF digi cluster collection for TMI18

c455a68

fix in cmssw master branch not propagated to p2l1pfp

c773b9b

jmitrevs reviewed Jan 21, 2026

View reviewed changes

cerminar reviewed Jan 22, 2026

View reviewed changes

cerminar requested a review from Copilot January 22, 2026 11:23

Copilot started reviewing on behalf of cerminar January 22, 2026 11:24 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

pallabidas added 3 commits February 2, 2026 22:07

PR comments from Jovan and Gianluca

4cb5464

geometry fixes for EG and PF cluster digis

36f44aa

bug fix in calo cluster phi values, integrate PF/had clusters into La…

52a058f

…yer-1 PF algo

pallabidas force-pushed the L1PF_15_1_X_GCTDigi branch from c1af17e to 52a058f Compare February 23, 2026 16:36

pallabidas added 2 commits February 23, 2026 19:29

changes to use TM18 in L1TCorrelatorLayer1Producer

57a3cd1

missed updates for TM18 collection

a42b8c8

pallabidas force-pushed the L1PF_15_1_X_GCTDigi branch from 6179d5c to a42b8c8 Compare February 26, 2026 10:27

jmitrevs mentioned this pull request Feb 26, 2026

CL1 TM18 version regionizer for barrel APx #152

Draft

		int crPhi = phi() % 5;
		if (phi() < 0) crPhi = (30 + phi()) % 5;

Conversation

pallabidas commented Jan 21, 2026

PR description:

Uh oh!

jmitrevs Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmitrevs Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmitrevs Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pallabidas Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmitrevs Jan 21, 2026 •

edited

Loading

jmitrevs Feb 2, 2026 •

edited

Loading

jmitrevs Feb 2, 2026 •

edited

Loading

pallabidas Feb 2, 2026 •

edited

Loading