From 519df2c6187b975c8bbe466aeb4f379fb869ced8 Mon Sep 17 00:00:00 2001 From: mmusich Date: Tue, 28 Nov 2023 15:31:06 +0100 Subject: [PATCH 1/9] add unit tests for PromptCalibProdSiStripLA workflow (worker+harvester) --- CalibTracker/SiStripLorentzAngle/test/BuildFile.xml | 1 + .../test/step_PromptCalibProdSiStripLA_cfg.py | 10 ++++++---- .../test/testPromptCalibProdSiStripLA.sh | 10 ++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 CalibTracker/SiStripLorentzAngle/test/BuildFile.xml create mode 100755 CalibTracker/SiStripLorentzAngle/test/testPromptCalibProdSiStripLA.sh diff --git a/CalibTracker/SiStripLorentzAngle/test/BuildFile.xml b/CalibTracker/SiStripLorentzAngle/test/BuildFile.xml new file mode 100644 index 0000000000000..b14cdc229877d --- /dev/null +++ b/CalibTracker/SiStripLorentzAngle/test/BuildFile.xml @@ -0,0 +1 @@ + diff --git a/CalibTracker/SiStripLorentzAngle/test/step_PromptCalibProdSiStripLA_cfg.py b/CalibTracker/SiStripLorentzAngle/test/step_PromptCalibProdSiStripLA_cfg.py index 821a59181417a..bb949a84c6362 100644 --- a/CalibTracker/SiStripLorentzAngle/test/step_PromptCalibProdSiStripLA_cfg.py +++ b/CalibTracker/SiStripLorentzAngle/test/step_PromptCalibProdSiStripLA_cfg.py @@ -13,6 +13,7 @@ process.load('Configuration.StandardSequences.Services_cff') process.load('SimGeneral.HepPDTESSource.pythiapdt_cfi') process.load('FWCore.MessageService.MessageLogger_cfi') +process.MessageLogger.cerr.FwkReport.reportEvery = 100 # limit the output for the unit test process.load('Configuration.EventContent.EventContentCosmics_cff') process.load('Configuration.StandardSequences.GeometryRecoDB_cff') process.load('Configuration.StandardSequences.MagneticField_cff') @@ -21,13 +22,13 @@ process.load('Configuration.StandardSequences.FrontierConditions_GlobalTag_cff') process.maxEvents = cms.untracked.PSet( - input = cms.untracked.int32(100000), + input = cms.untracked.int32(1000), # 1000000 output = cms.optional.untracked.allowed(cms.int32,cms.PSet) ) # Input source process.source = cms.Source("PoolSource", - fileNames = cms.untracked.vstring('/store/data/Commissioning2023/Cosmics/ALCARECO/SiStripCalCosmics-PromptReco-v1/000/364/141/00000/062e670e-40e3-4950-b0bb-dd354844d16f.root'), + fileNames = cms.untracked.vstring('/store/data/Commissioning2023/Cosmics/ALCARECO/SiStripCalCosmics-PromptReco-v1/000/364/174/00000/59a465b4-6e25-4ea0-8fe3-2319bdea7fcb.root'), secondaryFileNames = cms.untracked.vstring() ) @@ -104,11 +105,12 @@ from PhysicsTools.PatAlgos.tools.helpers import associatePatAlgosToolsTask associatePatAlgosToolsTask(process) -#Setup FWK for multithreaded +# Setup FWK for multithreaded process.options.numberOfThreads = 4 process.options.numberOfStreams = 0 - +# Save the per-histogram modules in order to test the SiStripHashedDetId +process.ALCARECOSiStripLACalib.saveHistoMods = cms.bool(True) # Customisation from command line diff --git a/CalibTracker/SiStripLorentzAngle/test/testPromptCalibProdSiStripLA.sh b/CalibTracker/SiStripLorentzAngle/test/testPromptCalibProdSiStripLA.sh new file mode 100755 index 0000000000000..53721c333e56b --- /dev/null +++ b/CalibTracker/SiStripLorentzAngle/test/testPromptCalibProdSiStripLA.sh @@ -0,0 +1,10 @@ +#!/bin/sh +function die { echo $1: status $2 ; exit $2; } + +# test worker +printf "TESTING SiStrip Lorentz Angle Worker ...\n\n" +cmsRun ${SCRAM_TEST_PATH}/step_PromptCalibProdSiStripLA_cfg.py || die "Failure running step_PromptCalibProdSiStripLA_cfg.py" $? + +# test harvester +printf "TESTING SiStrip Lorentz Angle Harvester ...\n\n" +cmsRun ${SCRAM_TEST_PATH}/step_PromptCalibProdSiStripLA_ALCAHARVEST_cfg.py || die "Failure running step_PromptCalibProdSiStripLA_ALCAHARVEST_cfg.py" $? From 6fb57ae3e7c60aac555f9717848ec110e882a9be Mon Sep 17 00:00:00 2001 From: mmusich Date: Wed, 29 Nov 2023 14:18:15 +0100 Subject: [PATCH 2/9] miscellaneous improvements to SiStripLorentzAnglePCLMonitor --- .../SiStripLorentzAngleCalibrationStruct.h | 2 +- .../plugins/SiStripLorentzAnglePCLMonitor.cc | 28 +++++++++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h b/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h index 6d6f74f58db98..1f7624b1fd7e8 100644 --- a/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h +++ b/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h @@ -28,7 +28,7 @@ struct SiStripLorentzAngleCalibrationHistograms { std::map p_; // These are vectors since std:map::find is expensive - // we're going to profi of the dense indexing offered by + // we're going to profit of the dense indexing offered by // SiStripHashedDetId and index the histogram position // with the natural booking order std::vector h2_ct_w_m_; diff --git a/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc b/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc index 56603db4c95c2..b17ef2c6b731d 100644 --- a/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc +++ b/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc @@ -187,13 +187,15 @@ void SiStripLorentzAnglePCLMonitor::dqmBeginRun(edm::Run const& run, edm::EventS m_hash = SiStripHashedDetId(c_rawid); //reserve the size of the vector - iHists_.h2_ct_w_m_.reserve(c_rawid.size()); - iHists_.h2_ct_var2_m_.reserve(c_rawid.size()); - iHists_.h2_ct_var3_m_.reserve(c_rawid.size()); + if (saveHistosMods_) { + iHists_.h2_ct_w_m_.reserve(c_rawid.size()); + iHists_.h2_ct_var2_m_.reserve(c_rawid.size()); + iHists_.h2_ct_var3_m_.reserve(c_rawid.size()); - iHists_.h2_t_w_m_.reserve(c_rawid.size()); - iHists_.h2_t_var2_m_.reserve(c_rawid.size()); - iHists_.h2_t_var3_m_.reserve(c_rawid.size()); + iHists_.h2_t_w_m_.reserve(c_rawid.size()); + iHists_.h2_t_var2_m_.reserve(c_rawid.size()); + iHists_.h2_t_var3_m_.reserve(c_rawid.size()); + } } std::string SiStripLorentzAnglePCLMonitor::moduleLocationType(const uint32_t& mod, const TrackerTopology* tTopo) { @@ -310,6 +312,11 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: const auto& hashedIndex = m_hash.hashedIndex(mod); + if (saveHistosMods_) { + LogDebug("SiStripLorentzAnglePCLMonitor") << "module ID: " << mod << " hashedIndex: " << hashedIndex; + iHists_.h1_["occupancyPerIndex"]->Fill(hashedIndex); + } + TVector3 localdir(c_localdirx, c_localdiry, c_localdirz); int sign = iHists_.orientation_[mod]; float tantheta = TMath::Tan(localdir.Theta()); @@ -348,6 +355,7 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: iHists_.h2_t_var3_m_[hashedIndex]->Fill(sign * cosphi * theta, c_variance); } } + // not in PCL if (saveHistosMods_) { iHists_.h2_ct_w_m_[hashedIndex]->Fill(sign * cosphi * tantheta, c_nstrips); @@ -392,6 +400,14 @@ void SiStripLorentzAnglePCLMonitor::bookHistograms(DQMStore::IBooker& ibook, "track_etaxchi2_2d", "track #chi^{2}/ndf vs track #eta;track #eta;track #chi^{2};tracks", 60, -3, 3, 100, 0, 5); // clang-format on + if (saveHistosMods_) { + iHists_.h1_["occupancyPerIndex"] = ibook.book1D("ClusterOccupancyPerHashedIndex", + "cluster occupancy;hashed index;# clusters per module", + m_hash.size(), + -0.5, + m_hash.size() - 0.5); + } + // fill in the module types iHists_.nlayers_["TIB"] = 4; iHists_.nlayers_["TOB"] = 6; From 349ac88c409f5942c5b8d43e5e1a9c2b2b2dfecd Mon Sep 17 00:00:00 2001 From: mmusich Date: Wed, 29 Nov 2023 14:20:28 +0100 Subject: [PATCH 3/9] add size() method for SiStripHashedDetId --- CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h b/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h index c1cc0e8644db5..2a02faee2abf6 100644 --- a/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h +++ b/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h @@ -59,6 +59,8 @@ class SiStripHashedDetId { inline const_iterator end() const; + inline const size_t size() const { return detIds_.size(); } + private: void init(const std::vector &); From f20b63ea62f70fa485b20a9f7e6a5b482805d93e Mon Sep 17 00:00:00 2001 From: mmusich Date: Wed, 29 Nov 2023 14:21:14 +0100 Subject: [PATCH 4/9] add catch2 test for SiStripHashedDetId --- .../SiStripObjects/test/BuildFile.xml | 7 ++ .../test/test_catch2_SiStripHashedDetId.cc | 106 ++++++++++++++++++ .../SiStripObjects/test/test_catch2_main.cc | 2 + 3 files changed, 115 insertions(+) create mode 100644 CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc create mode 100644 CalibFormats/SiStripObjects/test/test_catch2_main.cc diff --git a/CalibFormats/SiStripObjects/test/BuildFile.xml b/CalibFormats/SiStripObjects/test/BuildFile.xml index c8787ef0b7f29..eade084ae7e5d 100644 --- a/CalibFormats/SiStripObjects/test/BuildFile.xml +++ b/CalibFormats/SiStripObjects/test/BuildFile.xml @@ -15,4 +15,11 @@ + + + + + + + diff --git a/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc new file mode 100644 index 0000000000000..4f701da4e38be --- /dev/null +++ b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc @@ -0,0 +1,106 @@ +#include "CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h" +#include "CalibFormats/SiStripObjects/interface/SiStripDetInfo.h" +#include "CalibTracker/SiStripCommon/interface/SiStripDetInfoFileReader.h" +#include "FWCore/ParameterSet/interface/FileInPath.h" +#include "catch.hpp" +#include + +TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { + //_____________________________________________________________ + SECTION("Check constructing SiStripHashedDetId from DetId list") { + const auto& detInfo = SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + const auto& detIds = detInfo.getAllDetIds(); + SiStripHashedDetId hash(detIds); + std::cout << "Successfully created hash!" << std::endl; + REQUIRE(true); + } + + //_____________________________________________________________ + SECTION("Check manipulating SiStripHashedDetId") { + const auto& detInfo = SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + const auto& dets = detInfo.getAllDetIds(); + SiStripHashedDetId hash(dets); + + // Retrieve hashed indices + std::vector hashes; + uint32_t istart = time(NULL); + hashes.clear(); + hashes.reserve(dets.size()); + std::vector::const_iterator idet = dets.begin(); + for (; idet != dets.end(); ++idet) { + hashes.push_back(hash.hashedIndex(*idet)); + } + + // Some debug + std::stringstream ss; + ss << "[testSiStripHashedDetId::" << __func__ << "]"; + std::vector::const_iterator ii = hashes.begin(); + uint16_t cntr1 = 0; + for (; ii != hashes.end(); ++ii) { + if (*ii == sistrip::invalid32_) { + cntr1++; + ss << std::endl << " Invalid index " << *ii; + continue; + } + uint32_t detid = hash.unhashIndex(*ii); + std::vector::const_iterator iter = find(dets.begin(), dets.end(), detid); + if (iter == dets.end()) { + cntr1++; + ss << std::endl << " Did not find value " << detid << " at index " << ii - hashes.begin() << " in vector!"; + } else if (*ii != static_cast(iter - dets.begin())) { + cntr1++; + ss << std::endl + << " Found same value " << detid << " at different indices " << *ii << " and " << iter - dets.begin(); + } + } + if (cntr1) { + ss << std::endl << " Found " << cntr1 << " incompatible values!"; + } else { + ss << " Found no incompatible values!"; + } + std::cout << ss.str() << std::endl; + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Processed " << hashes.size() << " DetIds in " << (time(NULL) - istart) + << " seconds" << std::endl; + + // Retrieve DetIds + std::vector detids; + uint32_t jstart = time(NULL); + // meaasurement! + detids.clear(); + detids.reserve(dets.size()); + for (uint16_t idet = 0; idet < dets.size(); ++idet) { + detids.push_back(hash.unhashIndex(idet)); + } + + // Some debug + std::stringstream sss; + sss << "[testSiStripHashedDetId::" << __func__ << "]"; + uint16_t cntr2 = 0; + std::vector::const_iterator iii = detids.begin(); + for (; iii != detids.end(); ++iii) { + if (*iii != dets.at(iii - detids.begin())) { + cntr2++; + sss << std::endl + << " Diff values " << *iii << " and " << dets.at(iii - detids.begin()) << " found at index " + << iii - detids.begin() << " "; + } + } + if (cntr2) { + sss << std::endl << " Found " << cntr2 << " incompatible values!"; + } else { + sss << " Found no incompatible values!"; + } + std::cout << sss.str() << std::endl; + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Processed " << detids.size() << " hashed indices in " << (time(NULL) - jstart) + << " seconds" << std::endl; + + REQUIRE(true); + } + + + +} diff --git a/CalibFormats/SiStripObjects/test/test_catch2_main.cc b/CalibFormats/SiStripObjects/test/test_catch2_main.cc new file mode 100644 index 0000000000000..e6d1d565b15c0 --- /dev/null +++ b/CalibFormats/SiStripObjects/test/test_catch2_main.cc @@ -0,0 +1,2 @@ +#define CATCH_CONFIG_MAIN // This tells Catch to provide a main() - only do this in one cpp file +#include "catch.hpp" From 5b7f10a452fbf65d483d5e21439550686dcac9c2 Mon Sep 17 00:00:00 2001 From: mmusich Date: Wed, 29 Nov 2023 18:48:55 +0100 Subject: [PATCH 5/9] fix SiStripHashedDetId copy constructor --- CalibFormats/SiStripObjects/src/SiStripHashedDetId.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/CalibFormats/SiStripObjects/src/SiStripHashedDetId.cc b/CalibFormats/SiStripObjects/src/SiStripHashedDetId.cc index 1e789185a7f1d..d8ab17408cd7c 100644 --- a/CalibFormats/SiStripObjects/src/SiStripHashedDetId.cc +++ b/CalibFormats/SiStripObjects/src/SiStripHashedDetId.cc @@ -36,8 +36,15 @@ SiStripHashedDetId::SiStripHashedDetId(const std::vector &det_ids) : detI SiStripHashedDetId::SiStripHashedDetId(const SiStripHashedDetId &input) : detIds_(), id_(0), iter_(detIds_.begin()) { LogTrace(mlCabling_) << "[SiStripHashedDetId::" << __func__ << "]" << " Constructing object..."; - detIds_.reserve(input.end() - input.begin()); - std::copy(input.begin(), input.end(), detIds_.begin()); + + // auxilliary vector to store the list of raw IDs + std::vector raw_ids; + raw_ids.reserve(input.size()); + + // Copy elements from input vector to detIds_ vector + std::copy(input.begin(), input.end(), std::back_inserter(raw_ids)); + + init(raw_ids); } // ----------------------------------------------------------------------------- From 8b2231d0ced69fe1e4ee4ceae71ff8c9b98fa7d5 Mon Sep 17 00:00:00 2001 From: mmusich Date: Wed, 29 Nov 2023 18:49:24 +0100 Subject: [PATCH 6/9] add more testing in the catch2 unit tests for SiStripHashedDetId --- .../test/test_catch2_SiStripHashedDetId.cc | 172 ++++++++++++++---- 1 file changed, 135 insertions(+), 37 deletions(-) diff --git a/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc index 4f701da4e38be..96fb9d90d410a 100644 --- a/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc +++ b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc @@ -3,22 +3,121 @@ #include "CalibTracker/SiStripCommon/interface/SiStripDetInfoFileReader.h" #include "FWCore/ParameterSet/interface/FileInPath.h" #include "catch.hpp" + +#include +#include +#include #include TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { //_____________________________________________________________ SECTION("Check constructing SiStripHashedDetId from DetId list") { - const auto& detInfo = SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + const auto& detInfo = + SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); const auto& detIds = detInfo.getAllDetIds(); SiStripHashedDetId hash(detIds); - std::cout << "Successfully created hash!" << std::endl; + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Successfully created hash!" << std::endl; REQUIRE(true); } //_____________________________________________________________ - SECTION("Check manipulating SiStripHashedDetId") { - const auto& detInfo = SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + SECTION("Check SiStripHashedDetId copy constructor") { + const auto& detInfo = + SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); const auto& dets = detInfo.getAllDetIds(); + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " dets.size(): " << dets.size() << std::endl; + + SiStripHashedDetId hash(dets); + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " hash.size(): " << hash.size() << std::endl; + + // Retrieve hashed indices + std::vector hashes; + hashes.clear(); + hashes.reserve(dets.size()); + for (const auto& idet : dets) { + hashes.push_back(hash.hashedIndex(idet)); + } + + std::sort(hashes.begin(), hashes.end()); + + SiStripHashedDetId hash2(hash); + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Successfully copied hash map!" << std::endl; + + // Retrieve hashed indices + std::vector hashes2; + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " hashs2.size(): " << hash2.size() << std::endl; + + hashes2.clear(); + hashes2.reserve(dets.size()); + for (const auto& idet : dets) { + hashes2.push_back(hash2.hashedIndex(idet)); + } + + std::sort(hashes2.begin(), hashes2.end()); + + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Successfully sorted second hash map!" << std::endl; + + // Convert vectors to sets for easy set operations + std::set set1(hashes.begin(), hashes.end()); + std::set set2(hashes2.begin(), hashes2.end()); + + std::vector diff1to2, diff2to1; + + // Find elements in vec1 that are not in vec2 + std::set_difference(set1.begin(), set1.end(), set2.begin(), set2.end(), std::inserter(diff1to2, diff1to2.begin())); + + // Find elements in vec2 that are not in vec1 + std::set_difference(set2.begin(), set2.end(), set1.begin(), set1.end(), std::inserter(diff2to1, diff2to1.begin())); + + // Output the differences + if (!diff1to2.empty()) { + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Elements in hash that are not in hash2: "; + for (const auto& elem : diff1to2) { + std::cout << elem << " "; + } + std::cout << std::endl; + } + + if (!diff2to1.empty()) { + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Elements in hash2 that are not in hash: "; + for (const auto& elem : diff2to1) { + std::cout << elem << " "; + } + std::cout << std::endl; + } + + REQUIRE(hashes == hashes2); + } + + //_____________________________________________________________ + SECTION("Check manipulating SiStripHashedDetId") { + const auto& detInfo = + SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + + const auto& unsortedDets = detInfo.getAllDetIds(); + + // unfortunately SiStripDetInfo::getAllDetIds() returns a const vector + // so in order to sory we're gonna need to copy it first + + std::vector dets; + dets.reserve(unsortedDets.size()); + std::copy(unsortedDets.begin(), unsortedDets.end(), std::back_inserter(dets)); + + // sort the vector of detIds (otherwise the test won't work!) + std::sort(dets.begin(), dets.end()); + SiStripHashedDetId hash(dets); // Retrieve hashed indices @@ -26,44 +125,44 @@ TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { uint32_t istart = time(NULL); hashes.clear(); hashes.reserve(dets.size()); - std::vector::const_iterator idet = dets.begin(); - for (; idet != dets.end(); ++idet) { - hashes.push_back(hash.hashedIndex(*idet)); + for (const auto& idet : dets) { + hashes.push_back(hash.hashedIndex(idet)); } - + // Some debug std::stringstream ss; ss << "[testSiStripHashedDetId::" << __func__ << "]"; - std::vector::const_iterator ii = hashes.begin(); uint16_t cntr1 = 0; - for (; ii != hashes.end(); ++ii) { - if (*ii == sistrip::invalid32_) { - cntr1++; - ss << std::endl << " Invalid index " << *ii; - continue; + for (const auto& ii : hashes) { + if (ii == sistrip::invalid32_) { + cntr1++; + ss << std::endl << " Invalid index " << ii; + continue; } - uint32_t detid = hash.unhashIndex(*ii); + uint32_t detid = hash.unhashIndex(ii); std::vector::const_iterator iter = find(dets.begin(), dets.end(), detid); if (iter == dets.end()) { - cntr1++; - ss << std::endl << " Did not find value " << detid << " at index " << ii - hashes.begin() << " in vector!"; - } else if (*ii != static_cast(iter - dets.begin())) { - cntr1++; - ss << std::endl - << " Found same value " << detid << " at different indices " << *ii << " and " << iter - dets.begin(); + cntr1++; + ss << std::endl << " Did not find value " << detid << " at index " << ii - *(hashes.begin()) << " in vector!"; + } else if (ii != static_cast(iter - dets.begin())) { + cntr1++; + ss << std::endl + << " Found same value " << detid << " at different indices " << ii << " and " << iter - dets.begin(); } } + if (cntr1) { ss << std::endl << " Found " << cntr1 << " incompatible values!"; } else { ss << " Found no incompatible values!"; } std::cout << ss.str() << std::endl; - + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" - << " Processed " << hashes.size() << " DetIds in " << (time(NULL) - istart) - << " seconds" << std::endl; - + << " Processed " << hashes.size() << " DetIds in " << (time(NULL) - istart) << " seconds" << std::endl; + + REQUIRE(cntr1 == 0); + // Retrieve DetIds std::vector detids; uint32_t jstart = time(NULL); @@ -73,7 +172,7 @@ TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { for (uint16_t idet = 0; idet < dets.size(); ++idet) { detids.push_back(hash.unhashIndex(idet)); } - + // Some debug std::stringstream sss; sss << "[testSiStripHashedDetId::" << __func__ << "]"; @@ -81,10 +180,10 @@ TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { std::vector::const_iterator iii = detids.begin(); for (; iii != detids.end(); ++iii) { if (*iii != dets.at(iii - detids.begin())) { - cntr2++; - sss << std::endl - << " Diff values " << *iii << " and " << dets.at(iii - detids.begin()) << " found at index " - << iii - detids.begin() << " "; + cntr2++; + sss << std::endl + << " Diff values " << *iii << " and " << dets.at(iii - detids.begin()) << " found at index " + << iii - detids.begin() << " "; } } if (cntr2) { @@ -93,14 +192,13 @@ TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { sss << " Found no incompatible values!"; } std::cout << sss.str() << std::endl; - + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" - << " Processed " << detids.size() << " hashed indices in " << (time(NULL) - jstart) - << " seconds" << std::endl; - + << " Processed " << detids.size() << " hashed indices in " << (time(NULL) - jstart) << " seconds" + << std::endl; + + REQUIRE(cntr2 == 0); + REQUIRE(true); } - - - } From 6e38f12da61200945f609f57af54b2b59890d685 Mon Sep 17 00:00:00 2001 From: mmusich Date: Fri, 1 Dec 2023 09:40:28 +0100 Subject: [PATCH 7/9] rework hashing function usage in SiStripLorentzAnglePCLMonitor and add more per-module histograms --- .../SiStripLorentzAngleCalibrationStruct.h | 3 ++ .../plugins/SiStripLorentzAnglePCLMonitor.cc | 33 ++++++++++++++----- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h b/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h index 1f7624b1fd7e8..bb99e599409c2 100644 --- a/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h +++ b/CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationStruct.h @@ -7,6 +7,7 @@ // user includes #include "DQMServices/Core/interface/DQMStore.h" +#include "CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h" struct SiStripLorentzAngleCalibrationHistograms { public: @@ -31,6 +32,8 @@ struct SiStripLorentzAngleCalibrationHistograms { // we're going to profit of the dense indexing offered by // SiStripHashedDetId and index the histogram position // with the natural booking order + SiStripHashedDetId hash_; + std::vector h2_ct_w_m_; std::vector h2_ct_var2_m_; std::vector h2_ct_var3_m_; diff --git a/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc b/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc index b17ef2c6b731d..22d983621fa52 100644 --- a/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc +++ b/CalibTracker/SiStripLorentzAngle/plugins/SiStripLorentzAnglePCLMonitor.cc @@ -22,7 +22,6 @@ #include // user include files -#include "CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h" #include "CalibTracker/Records/interface/SiStripDependentRecords.h" #include "CalibTracker/SiStripCommon/interface/ShallowTools.h" #include "CalibTracker/SiStripLorentzAngle/interface/SiStripLorentzAngleCalibrationHelpers.h" @@ -74,7 +73,6 @@ class SiStripLorentzAnglePCLMonitor : public DQMEDAnalyzer { // ------------ member data ------------ SiStripClusterInfo m_clusterInfo; SiStripLorentzAngleCalibrationHistograms iHists_; - SiStripHashedDetId m_hash; // for magnetic field conversion static constexpr float teslaToInverseGeV_ = 2.99792458e-3f; @@ -183,8 +181,11 @@ void SiStripLorentzAnglePCLMonitor::dqmBeginRun(edm::Run const& run, edm::EventS // Sorted DetId list gives max performance, anything else is worse std::sort(c_rawid.begin(), c_rawid.end()); - // initialized the hash map - m_hash = SiStripHashedDetId(c_rawid); + // initialize the hash map + // in case it's not already initialized + if (iHists_.hash_.size() == 0) { + iHists_.hash_ = SiStripHashedDetId(c_rawid); + } //reserve the size of the vector if (saveHistosMods_) { @@ -310,7 +311,8 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: if (locationtype.empty()) return; - const auto& hashedIndex = m_hash.hashedIndex(mod); + // retrive the hashed index + const auto& hashedIndex = iHists_.hash_.hashedIndex(mod); if (saveHistosMods_) { LogDebug("SiStripLorentzAnglePCLMonitor") << "module ID: " << mod << " hashedIndex: " << hashedIndex; @@ -331,6 +333,12 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: iHists_.h2_[Form("%s_tanthcosphtrk_nstrip", locationtype.c_str())]->Fill(sign * cosphi * tantheta, c_nstrips); iHists_.h2_[Form("%s_thetatrk_nstrip", locationtype.c_str())]->Fill(sign * theta * cosphi, c_nstrips); + if (saveHistosMods_) { + iHists_.h1_[Form("%s_%d_nstrips", locationtype.c_str(), mod)]->Fill(c_nstrips); + iHists_.h1_[Form("%s_%d_tanthetatrk", locationtype.c_str(), mod)]->Fill(sign * tantheta); + iHists_.h1_[Form("%s_%d_cosphitrk", locationtype.c_str(), mod)]->Fill(cosphi); + } + // variance for width == 2 if (c_nstrips == 2) { iHists_.h1_[Form("%s_variance_w2", locationtype.c_str())]->Fill(c_variance); @@ -339,6 +347,8 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: // not in PCL if (saveHistosMods_) { + LogDebug("SiStripLorentzAnglePCLMonitor") << iHists_.h2_ct_var2_m_[hashedIndex]->getName(); + iHists_.h1_[Form("%s_%d_variance_w2", locationtype.c_str(), mod)]->Fill(c_variance); iHists_.h2_ct_var2_m_[hashedIndex]->Fill(sign * cosphi * tantheta, c_variance); iHists_.h2_t_var2_m_[hashedIndex]->Fill(sign * cosphi * theta, c_variance); } @@ -351,6 +361,7 @@ void SiStripLorentzAnglePCLMonitor::analyze(const edm::Event& iEvent, const edm: // not in PCL if (saveHistosMods_) { + iHists_.h1_[Form("%s_%d_variance_w3", locationtype.c_str(), mod)]->Fill(c_variance); iHists_.h2_ct_var3_m_[hashedIndex]->Fill(sign * cosphi * tantheta, c_variance); iHists_.h2_t_var3_m_[hashedIndex]->Fill(sign * cosphi * theta, c_variance); } @@ -403,9 +414,9 @@ void SiStripLorentzAnglePCLMonitor::bookHistograms(DQMStore::IBooker& ibook, if (saveHistosMods_) { iHists_.h1_["occupancyPerIndex"] = ibook.book1D("ClusterOccupancyPerHashedIndex", "cluster occupancy;hashed index;# clusters per module", - m_hash.size(), + iHists_.hash_.size(), -0.5, - m_hash.size() - 0.5); + iHists_.hash_.size() - 0.5); } // fill in the module types @@ -467,6 +478,7 @@ void SiStripLorentzAnglePCLMonitor::bookHistograms(DQMStore::IBooker& ibook, if (saveHistosMods_) { ibook.setCurrentFolder(folderToBook + "/modules"); for (const auto& [mod, locationType] : iHists_.moduleLocationType_) { + ibook.setCurrentFolder(folderToBook + "/modules" + Form("/%s", locationType.c_str())); // histograms for each module iHists_.h1_[Form("%s_%d_nstrips", locationType.c_str(), mod)] = ibook.book1D(Form("%s_%d_nstrips", locationType.c_str(), mod), "", 10, 0, 10); @@ -481,9 +493,12 @@ void SiStripLorentzAnglePCLMonitor::bookHistograms(DQMStore::IBooker& ibook, } int counter{0}; - SiStripHashedDetId::const_iterator iter = m_hash.begin(); - for (; iter != m_hash.end(); ++iter) { + SiStripHashedDetId::const_iterator iter = iHists_.hash_.begin(); + for (; iter != iHists_.hash_.end(); ++iter) { + LogDebug("SiStripLorentzAnglePCLMonitor") + << "detId: " << (*iter) << " hashed index: " << iHists_.hash_.hashedIndex((*iter)); const auto& locationType = iHists_.moduleLocationType_[(*iter)]; + ibook.setCurrentFolder(folderToBook + "/modules" + Form("/%s", locationType.c_str())); iHists_.h2_ct_w_m_.push_back( ibook.book2D(Form("ct_w_m_%s_%d", locationType.c_str(), *iter), "", 90, -0.9, 0.9, 10, 0, 10)); iHists_.h2_t_w_m_.push_back( From 3eda318fba5d66d4c3ec53013a49c53fd874dc31 Mon Sep 17 00:00:00 2001 From: mmusich Date: Fri, 1 Dec 2023 09:40:57 +0100 Subject: [PATCH 8/9] fix SiStripHashedDetId assignment operator --- .../interface/SiStripHashedDetId.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h b/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h index 2a02faee2abf6..96e11bc09818d 100644 --- a/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h +++ b/CalibFormats/SiStripObjects/interface/SiStripHashedDetId.h @@ -30,7 +30,21 @@ class SiStripHashedDetId { SiStripHashedDetId(const SiStripHashedDetId &); /** Assignment operator. */ - SiStripHashedDetId &operator=(const SiStripHashedDetId &) = default; + SiStripHashedDetId &operator=(const SiStripHashedDetId &other) { + if (this != &other) { // Self-assignment check + this->id_ = 0; + this->iter_ = other.begin(); + // auxilliary vector to store the list of raw IDs + std::vector raw_ids; + raw_ids.reserve(other.size()); + + // Copy elements from input vector to detIds_ vector + std::copy(other.begin(), other.end(), std::back_inserter(raw_ids)); + + this->init(raw_ids); + } + return *this; + } /** Public default constructor. */ SiStripHashedDetId(); From 5f34bdd3b21c6b2dceb59a618acc965a08e70252 Mon Sep 17 00:00:00 2001 From: mmusich Date: Fri, 1 Dec 2023 11:09:51 +0100 Subject: [PATCH 9/9] add unit test for SiStripHashedDetId assignment operator --- .../test/test_catch2_SiStripHashedDetId.cc | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc index 96fb9d90d410a..1b5cae7703782 100644 --- a/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc +++ b/CalibFormats/SiStripObjects/test/test_catch2_SiStripHashedDetId.cc @@ -101,6 +101,49 @@ TEST_CASE("SiStripHashedDetId testing", "[SiStripHashedDetId]") { REQUIRE(hashes == hashes2); } + //_____________________________________________________________ + SECTION("Check SiStripHashedDetId assignment operator") { + const auto& detInfo = + SiStripDetInfoFileReader::read(edm::FileInPath(SiStripDetInfoFileReader::kDefaultFile).fullPath()); + const auto& dets = detInfo.getAllDetIds(); + + SiStripHashedDetId hash(dets); + SiStripHashedDetId hash2; + + // Retrieve hashed indices + std::vector hashes; + hashes.clear(); + hashes.reserve(dets.size()); + for (const auto& idet : dets) { + hashes.push_back(hash.hashedIndex(idet)); + } + + std::sort(hashes.begin(), hashes.end()); + + // assign hash to hash2 + hash2 = hash; + + // Retrieve hashed indices + std::vector hashes2; + hashes2.clear(); + hashes2.reserve(dets.size()); + for (const auto& idet : dets) { + hashes2.push_back(hash2.hashedIndex(idet)); + } + + std::sort(hashes2.begin(), hashes2.end()); + + if (hashes == hashes2) { + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Assigned SiStripHashedDetId matches original one!" << std::endl; + } else { + std::cout << "[testSiStripHashedDetId::" << __func__ << "]" + << " Assigned SiStripHashedDetId does not match the original one!" << std::endl; + } + + REQUIRE(hashes == hashes2); + } + //_____________________________________________________________ SECTION("Check manipulating SiStripHashedDetId") { const auto& detInfo =