Merge pull request #33737 from Dr15Jones/fixEcalPulseSymmCovariance

Fixed indexing error in EcalPulseSymmCovariance
cms-sw · May 24, 2021 · c628f33 · c628f33
2 parents 493c90b + 292e51a
commit c628f33
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 13 deletions.
diff --git a/CondFormats/EcalObjects/interface/EcalPulseSymmCovariances.h b/CondFormats/EcalObjects/interface/EcalPulseSymmCovariances.h
@@ -6,21 +6,23 @@
 #include "CondFormats/EcalObjects/interface/EcalPulseShapes.h"
 #include "CondFormats/EcalObjects/interface/EcalCondObjectContainer.h"
 
+#include <algorithm>
+
 struct EcalPulseSymmCovariance {
 public:
   EcalPulseSymmCovariance();
 
   float covval[EcalPulseShape::TEMPLATESAMPLES * (EcalPulseShape::TEMPLATESAMPLES + 1) / 2];
 
-  float val(int i, int j) const {
-    int k = -1;
-    if (j >= i)
-      k = j + (EcalPulseShape::TEMPLATESAMPLES - 1) * i;
-    else
-      k = i + (EcalPulseShape::TEMPLATESAMPLES - 1) * j;
-    return covval[k];
+  int indexFor(int i, int j) const {
+    int m = std::min(i, j);
+    int n = std::max(i, j);
+    return n + EcalPulseShape::TEMPLATESAMPLES * m - m * (m + 1) / 2;
   }
 
+  float val(int i, int j) const { return covval[indexFor(i, j)]; }
+  float& val(int i, int j) { return covval[indexFor(i, j)]; }
+
   COND_SERIALIZABLE;
 };
 

diff --git a/CondFormats/EcalObjects/test/BuildFile.xml b/CondFormats/EcalObjects/test/BuildFile.xml
@@ -14,3 +14,6 @@
 </environment>
 <bin file="testSerializationEcalObjects.cpp">
 </bin>
+<bin file="EcalPulseSymmCovariance_catch2.cc">
+  <use name="catch2"/>
+</bin>
diff --git a/CondFormats/EcalObjects/test/EcalPulseSymmCovariance_catch2.cc b/CondFormats/EcalObjects/test/EcalPulseSymmCovariance_catch2.cc
@@ -0,0 +1,70 @@
+#include "CondFormats/EcalObjects/interface/EcalPulseSymmCovariances.h"
+
+#define CATCH_CONFIG_MAIN  // This tells Catch to provide a main() - only do this in one cpp file
+#include "catch.hpp"
+#include <iostream>
+
+TEST_CASE("EcalPulseSymmCovariance testing", "[EcalPulseSymmCovariance]") {
+  EcalPulseSymmCovariance covMutable;
+  //fill with accending values
+  float const vMin = 0.5f;
+  float v = vMin;
+
+  std::set<float> values;
+  for (auto& entry : covMutable.covval) {
+    entry = v;
+    values.insert(v);
+    v += 1.f;
+  }
+
+  float const vMax = v - 1.f;
+
+  auto const& cov = covMutable;
+
+  SECTION("Check symmetry") {
+    for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i) {
+      for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
+        REQUIRE(cov.val(i, j) == cov.val(j, i));
+      }
+    }
+  }
+  SECTION("Check index coverage") {
+    std::vector<bool> hitIndices(std::size(cov.covval), false);
+    for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i) {
+      for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
+        hitIndices[cov.indexFor(i, j)] = true;
+      }
+    }
+    for (auto indx : hitIndices) {
+      REQUIRE(indx == true);
+    }
+  }
+  SECTION("Check bounds") {
+    for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i) {
+      for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
+        REQUIRE(cov.indexFor(i, j) < std::size(cov.covval));
+      }
+    }
+  }
+
+  SECTION("Check known values") {
+    for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i) {
+      for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
+        REQUIRE(vMin <= cov.val(i, j));
+        REQUIRE(cov.val(i, j) <= vMax);
+        REQUIRE(values.end() != values.find(cov.val(i, j)));
+      }
+    }
+  }
+
+  SECTION("Check filling") {
+    float v = vMin;
+    EcalPulseSymmCovariance covNew;
+    for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i) {
+      for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
+        covNew.val(i, j) = v;
+        REQUIRE(v == covNew.val(i, j));
+      }
+    }
+  }
+}
diff --git a/CondTools/Ecal/src/EcalPulseSymmCovariancesHandler.cc b/CondTools/Ecal/src/EcalPulseSymmCovariancesHandler.cc
@@ -76,12 +76,7 @@ void popcon::EcalPulseSymmCovariancesHandler::getNewObjects() {
       EcalPulseSymmCovariances::Item item;
       for (int i = 0; i < EcalPulseShape::TEMPLATESAMPLES; ++i)
         for (int j = 0; j < EcalPulseShape::TEMPLATESAMPLES; ++j) {
-          int k = -1;
-          if (j >= i)
-            k = j + (EcalPulseShape::TEMPLATESAMPLES - 1) * i;
-          else
-            k = i + (EcalPulseShape::TEMPLATESAMPLES - 1) * j;
-          item.covval[k] = covvals[i][j];
+          item.val(i, j) = covvals[i][j];
         }
 
       if (isbarrel) {