Address review comments

Author: cerminar

DataFormats/L1TParticleFlow/interface/PFCluster.h

@@ -40,12 +40,12 @@ namespace l1t {
       setPdgId(isEM ? 22 : 130);  // photon : non-photon(K0)
     }
 
-    enum class HWEncoding { None = 0, Had = 1, Em = 2, Gct = 3 };
+    enum class HWEncoding { None = 0, Had = 1, Em = 2 };
 
     float hOverE() const { return hOverE_; }
     void setHOverE(float hOverE) { hOverE_ = hOverE; }
 
-    // FIXME: this might nor be consistent with the value stored in the HW digi
+    // NOTE: this might not be consistent with the value stored in the HW digi
     float emEt() const {
       if (hOverE_ == -1)
         return 0;

DataFormats/L1TParticleFlow/interface/datatypes.h

@@ -157,6 +157,7 @@ namespace l1ct {
     constexpr float SRRTOT_LSB = 0.0019531250;  // pow(2, -9)
     constexpr unsigned int SRRTOT_SCALE = 64;   // pow(2, 6)
     constexpr float HOE_LSB = 0.031250000;      // pow(2, -5)
+    // empirical choice: we saturate to 16 while waiting for updates to the GCT-CTL1 interface document
     constexpr unsigned int RELISO_SCALE = 16;
 
     inline float floatPt(pt_t pt) { return pt.to_float(); }

L1Trigger/Phase2L1ParticleFlow/interface/conifer.h

@@ -6,9 +6,7 @@
 #include "DataFormats/L1TParticleFlow/interface/pf.h"
 #include "L1Trigger/Phase2L1ParticleFlow/interface/common/inversion.h"
 
-// FIXME: back to the old way of including conifer.h
-#include "L1Trigger/Phase2L1ParticleFlow/interface/conifer.h"
-// #include "conifer.h"
+#include "conifer.h"
 
 namespace edm {
   class ParameterSet;

L1Trigger/Phase2L1ParticleFlow/interface/egamma/pftkegalgo_ref.h

@@ -33,7 +33,7 @@ namespace l1ct {
     ap_int<7> phi(const ap_uint<64> &in) const { return (ap_int<7>)in.range(25, 19); }
 
     // HoE value
-    ap_uint<4> hoe(const ap_uint<64> &in) const { return in.range(30, 26); }
+    ap_uint<4> hoe(const ap_uint<64> &in) const { return in.range(29, 26); }
   };
 }  // namespace l1ct
 

L1Trigger/Phase2L1ParticleFlow/interface/l1-converters/gcthadinput_ref.h

@@ -3,9 +3,7 @@
 
 #include "DataFormats/L1TParticleFlow/interface/layer1_emulator.h"
 #include "L1Trigger/Phase2L1ParticleFlow/interface/corrector.h"
-// FIXME: move to the include from external package
-#include "L1Trigger/Phase2L1ParticleFlow/interface/conifer.h"
-// #include "conifer.h"
+#include "conifer.h"
 
 namespace edm {
   class ParameterSet;
@@ -67,7 +65,7 @@ namespace l1ct {
         typedef activation_inv_table_t inv_table_t;
       };
 
-      conifer::BDT<bdt_feature_t, bdt_score_t, false> *multiclass_bdt_;
+      std::unique_ptr<conifer::BDT<bdt_feature_t, bdt_score_t, false>> multiclass_bdt_;
     };
 
     enum class UseEmInterp { No, EmOnly, AllKeepHad, AllKeepTot };

L1Trigger/Phase2L1ParticleFlow/interface/l1-converters/hgcalinput_ref.h

@@ -746,10 +746,10 @@ void L1TCorrelatorLayer1Producer::rawHgcalClusterEncode(ap_uint<256> &cwrd,
   // FIXME: we keep subtracting an arbitrary number different from the HLGCal FW one
   ap_ufixed<12, 11, AP_RND_CONV, AP_SAT> w_meanz = fabs(c.zBarycenter()) - 320;  // LSB = 0.5cm
 
-  ap_uint<6> w_showerlenght = c.showerLength();
+  ap_uint<6> w_showerlength = c.showerLength();
   ap_uint<7> w_sigmazz = round(c.sigmaZZ() / SIGMAZZ_LSB);
   ap_uint<7> w_sigmaphiphi = round(c.sigmaPhiPhiTot() / SIGMAPHIPHI_LSB);
-  ap_uint<6> w_coreshowerlenght = c.coreShowerLength();
+  ap_uint<6> w_coreshowerlength = c.coreShowerLength();
   ap_uint<5> w_sigmaetaeta = round(c.sigmaEtaEtaTot() / SIGMAETAETA_LSB);
   // NOTE: this is an arbitrary choice to keep the rounding consistent with the "addDecodedHadCalo" one
   // FIXME: the scaling here is added to the encoded word...
@@ -766,10 +766,10 @@ void L1TCorrelatorLayer1Producer::rawHgcalClusterEncode(ap_uint<256> &cwrd,
   cwrd(64 + 18, 64 + 10) = w_phi;            //  9 bits: 18-10
   cwrd(64 + 30, 64 + 19) = w_meanz.range();  // 12 bits: 30-19
   // Word 2
-  cwrd(128 + 18, 128 + 13) = w_showerlenght;      //  6 bits: 18-13
+  cwrd(128 + 18, 128 + 13) = w_showerlength;      //  6 bits: 18-13
   cwrd(128 + 38, 128 + 32) = w_sigmazz;           //  7 bits: 38-32
   cwrd(128 + 45, 128 + 39) = w_sigmaphiphi;       //  7 bits: 45-39
-  cwrd(128 + 51, 128 + 46) = w_coreshowerlenght;  //  6 bits: 51-46
+  cwrd(128 + 51, 128 + 46) = w_coreshowerlength;  //  6 bits: 51-46
   cwrd(128 + 56, 128 + 52) = w_sigmaetaeta;       //  5 bits: 56-52
 
   // cwrd(128+63, 128+57) = w_sigmarrtot;       //  7 bits: 63-57 // FIXME: use word3 spare bits

L1Trigger/Phase2L1ParticleFlow/plugins/L1TCorrelatorLayer1Producer.cc

@@ -271,8 +271,9 @@ float l1tpf::corrector::correctedPt(float pt, float emPt, float eta) const {
     return total;  // no correction
   ieta = std::min(std::max<unsigned>(1, index_->GetXaxis()->FindBin(abseta)), neta_) - 1;
   // FIXME: why eta 3.1 is hardcoded here?
+  static const float maxeta = 3.1;
   unsigned int iemf =
-      is2d_ && abseta < 3.1 ? std::min(std::max<unsigned>(1, index_->GetYaxis()->FindBin(emf)), nemf_) - 1 : 0;
+      is2d_ && abseta < maxeta ? std::min(std::max<unsigned>(1, index_->GetYaxis()->FindBin(emf)), nemf_) - 1 : 0;
   float ptcorr = 0;
   if (!emulate_) {  // not emulation - read from the TGraph as normal
     TGraph *graph = corrections_[ieta * nemf_ + iemf];

L1Trigger/Phase2L1ParticleFlow/src/corrector.cc

@@ -109,8 +109,6 @@ l1ct::HadCaloObjEmu l1ct::HgcalClusterDecoderEmulator::decode(const l1ct::PFRegi
 
   // Conversion to local (input sector) coordinates
   ap_int<9> w_eta = l1ct::glbeta_t(w_abseta.to_int() * (sector.floatEtaCenter() > 0 ? +1 : -1)) - sector.hwEtaCenter;
-  // if (sector.floatEtaCenter() > 0)
-  //   w_phi = -w_phi;
 
   l1ct::HadCaloObjEmu out;
   out.clear();
@@ -202,7 +200,7 @@ l1ct::HgcalClusterDecoderEmulator::MultiClassID::MultiClassID(const std::string
 #else
   auto resolvedFileName = model;
 #endif
-  multiclass_bdt_ = new conifer::BDT<bdt_feature_t, bdt_score_t, false>(resolvedFileName);
+  multiclass_bdt_ = std::make_unique<conifer::BDT<bdt_feature_t, bdt_score_t, false>>(resolvedFileName);
 }
 
 bool l1ct::HgcalClusterDecoderEmulator::MultiClassID::evaluate(l1ct::HadCaloObjEmu &cl,