fix: use unique_ptr for geometry in DD4hep_service, hand out not_null…

… const pointers (#1045) ### Briefly, what does this PR introduce? While looking into #1032, #1035, #1036 I started realizing two things: - cell ID position converters are created by algorithms from geometry instead of doing this once in the service, - there is no const protection in the detector pointers passed to algorithms (i.e. an algorithm could change geometry), - there is no protection against null pointers being passed from geometry. The first two points are related since the interface for creating a cell ID position converter requires a non-const detector (see also AIDASoft/DD4hep#1148). This PR centralizes the creation of cell ID position converters, clarifies ownership through unique_ptrs, and makes sure no null pointers are passed to the algorithms. In places where the constructor gets the const not_null pointer, we can also impose this in the class on the relevant member variable. ### What kind of change does this PR introduce? - [x] Bug fix (issue: avoid creation of cell ID position converters in algorithms) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [x] Changes have been communicated to collaborators: - @simonge reordered MatrixTransferStatic init arguments - @c-dilks @chchatte92 this also affects RichGeo_service etc. ### Does this PR introduce breaking changes? What changes might users need to make to their code? Yes, now requires GSL (which has always been in eic-shell and is a requirement). ### Does this PR change default behavior? No. --------- Co-authored-by: Christopher Dilks <[email protected]> Co-authored-by: Dmitry Kalinkin <[email protected]>
eic · Oct 5, 2023 · 0596722 · 0596722
1 parent aef9eca
commit 0596722
Show file tree

Hide file tree

Showing 40 changed files with 138 additions and 148 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -116,6 +116,9 @@ find_package(podio REQUIRED)
 find_package(EDM4HEP REQUIRED)
 find_package(EDM4EIC 2.1 REQUIRED)
 
+# Guidelines Support Library
+find_package(Microsoft.GSL CONFIG)
+
 # Remove PODIO_JSON_OUTPUT (ref: https://github.com/AIDASoft/podio/issues/475)
 get_target_property(EDM4HEP_INTERFACE_COMPILE_DEFINITIONS EDM4HEP::edm4hep INTERFACE_COMPILE_DEFINITIONS)
 if(EDM4HEP_INTERFACE_COMPILE_DEFINITIONS)

diff --git a/src/algorithms/calorimetry/CalorimeterHitReco.cc b/src/algorithms/calorimetry/CalorimeterHitReco.cc
@@ -15,9 +15,9 @@ using namespace dd4hep;
 
 namespace eicrecon {
 
-void CalorimeterHitReco::init(const dd4hep::Detector* detector, std::shared_ptr<spdlog::logger>& logger) {
+void CalorimeterHitReco::init(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger) {
     m_detector = detector;
-    m_converter = std::make_shared<const dd4hep::rec::CellIDPositionConverter>(const_cast<dd4hep::Detector&>(*detector));
+    m_converter = converter;
     m_log = logger;
 
     // threshold for firing

diff --git a/src/algorithms/calorimetry/CalorimeterHitReco.h b/src/algorithms/calorimetry/CalorimeterHitReco.h
@@ -24,7 +24,7 @@ namespace eicrecon {
   class CalorimeterHitReco : public WithPodConfig<CalorimeterHitRecoConfig> {
 
   public:
-    void init(const dd4hep::Detector* detector, std::shared_ptr<spdlog::logger>& logger);
+    void init(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger);
     std::unique_ptr<edm4eic::CalorimeterHitCollection> process(const edm4hep::RawCalorimeterHitCollection &rawhits);
 
   private:
@@ -46,7 +46,7 @@ namespace eicrecon {
 
   private:
     const dd4hep::Detector* m_detector;
-    std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_converter;
+    const dd4hep::rec::CellIDPositionConverter* m_converter;
     std::shared_ptr<spdlog::logger> m_log;
 
   };

diff --git a/src/algorithms/calorimetry/CalorimeterHitsMerger.cc b/src/algorithms/calorimetry/CalorimeterHitsMerger.cc
@@ -12,9 +12,9 @@
 
 namespace eicrecon {
 
-void CalorimeterHitsMerger::init(const dd4hep::Detector* detector, std::shared_ptr<spdlog::logger>& logger) {
+void CalorimeterHitsMerger::init(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger) {
     m_detector = detector;
-    m_converter = std::make_shared<const dd4hep::rec::CellIDPositionConverter>(const_cast<dd4hep::Detector&>(*detector));
+    m_converter = converter;
     m_log = logger;
 
     if (m_cfg.readout.empty()) {

diff --git a/src/algorithms/calorimetry/CalorimeterHitsMerger.h b/src/algorithms/calorimetry/CalorimeterHitsMerger.h
@@ -31,15 +31,15 @@ namespace eicrecon {
   class CalorimeterHitsMerger : public WithPodConfig<CalorimeterHitsMergerConfig>  {
 
   public:
-    void init(const dd4hep::Detector* detector, std::shared_ptr<spdlog::logger>& logger);
+    void init(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger);
     std::unique_ptr<edm4eic::CalorimeterHitCollection> process(const edm4eic::CalorimeterHitCollection &input);
 
   private:
     uint64_t id_mask{0}, ref_mask{0};
 
   private:
     const dd4hep::Detector* m_detector;
-    std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_converter;
+    const dd4hep::rec::CellIDPositionConverter* m_converter;
     std::shared_ptr<spdlog::logger> m_log;
 
   };

diff --git a/src/algorithms/digi/PhotoMultiplierHitDigi.cc b/src/algorithms/digi/PhotoMultiplierHitDigi.cc
@@ -18,12 +18,12 @@
 //------------------------
 // AlgorithmInit
 //------------------------
-void eicrecon::PhotoMultiplierHitDigi::AlgorithmInit(dd4hep::Detector *detector, std::shared_ptr<spdlog::logger>& logger)
+void eicrecon::PhotoMultiplierHitDigi::AlgorithmInit(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger)
 {
     // services
     m_detector = detector;
-    m_cellid_converter = std::make_shared<const dd4hep::rec::CellIDPositionConverter>(*detector);
-    m_log=logger;
+    m_converter = converter;
+    m_log = logger;
 
     // print the configuration parameters
     m_cfg.Print(m_log, spdlog::level::debug);
@@ -132,7 +132,7 @@ eicrecon::PhotoMultiplierHitDigiResult eicrecon::PhotoMultiplierHitDigi::Algorit
             // cell time, signal amplitude
             double   amp  = m_cfg.speMean + m_rngNorm()*m_cfg.speError;
             TimeType time = m_cfg.noiseTimeWindow*m_rngUni() / dd4hep::ns;
-            dd4hep::Position pos_hit_global = m_cellid_converter->position(id);
+            dd4hep::Position pos_hit_global = m_converter->position(id);
 
             // insert in `hit_groups`, or if the pixel already has a hit, update `npe` and `signal`
             this->InsertHit(

diff --git a/src/algorithms/digi/PhotoMultiplierHitDigi.h b/src/algorithms/digi/PhotoMultiplierHitDigi.h
@@ -41,7 +41,7 @@ class PhotoMultiplierHitDigi : public WithPodConfig<PhotoMultiplierHitDigiConfig
 public:
     PhotoMultiplierHitDigi() = default;
     ~PhotoMultiplierHitDigi(){}
-    void AlgorithmInit(dd4hep::Detector *detector, std::shared_ptr<spdlog::logger>& logger);
+    void AlgorithmInit(const dd4hep::Detector* detector, const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger);
     void AlgorithmChangeRun();
     PhotoMultiplierHitDigiResult AlgorithmProcess(
         const edm4hep::SimTrackerHitCollection* sim_hits
@@ -106,10 +106,10 @@ class PhotoMultiplierHitDigi : public WithPodConfig<PhotoMultiplierHitDigiConfig
         bool             is_noise_hit = false
         );
 
-    dd4hep::Detector *m_detector   = nullptr;
+    const dd4hep::Detector* m_detector = nullptr;
+    const dd4hep::rec::CellIDPositionConverter* m_converter;
 
     std::shared_ptr<spdlog::logger> m_log;
-    std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_cellid_converter;
 
     // std::default_random_engine generator; // TODO: need something more appropriate here
     // std::normal_distribution<double> m_normDist; // defaults to mean=0, sigma=1

diff --git a/src/algorithms/fardetectors/MatrixTransferStatic.cc b/src/algorithms/fardetectors/MatrixTransferStatic.cc
@@ -5,13 +5,13 @@
 
 #include "MatrixTransferStatic.h"
 
-void eicrecon::MatrixTransferStatic::init(std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> id_conv,
-                                          const dd4hep::Detector* det,
+void eicrecon::MatrixTransferStatic::init(const dd4hep::Detector* det,
+                                          const dd4hep::rec::CellIDPositionConverter* id_conv,
                                           std::shared_ptr<spdlog::logger> &logger) {
 
-  m_log              = logger;
-  m_detector         = det;
-  m_cellid_converter = id_conv;
+  m_log       = logger;
+  m_detector  = det;
+  m_converter = id_conv;
   //Calculate inverse of static transfer matrix
   std::vector<std::vector<double>> aX(m_cfg.aX);
   std::vector<std::vector<double>> aY(m_cfg.aY);
@@ -64,7 +64,7 @@ std::unique_ptr<edm4eic::ReconstructedParticleCollection> eicrecon::MatrixTransf
 
     auto cellID = h.getCellID();
     // The actual hit position in Global Coordinates
-    auto gpos = m_cellid_converter->position(cellID);
+    auto gpos = m_converter->position(cellID);
     // local positions
     auto volman = m_detector->volumeManager();
     auto local = volman.lookupDetElement(cellID);

diff --git a/src/algorithms/fardetectors/MatrixTransferStatic.h b/src/algorithms/fardetectors/MatrixTransferStatic.h
@@ -26,7 +26,7 @@ namespace eicrecon {
     double aYinv[2][2] = {{0.0, 0.0},
                           {0.0, 0.0}};
 
-    void init(const std::shared_ptr<const dd4hep::rec::CellIDPositionConverter>,const dd4hep::Detector* det,std::shared_ptr<spdlog::logger> &logger);
+    void init(const dd4hep::Detector* det, const dd4hep::rec::CellIDPositionConverter* id_conv, std::shared_ptr<spdlog::logger> &logger);
 
     std::unique_ptr<edm4eic::ReconstructedParticleCollection> produce(const edm4hep::SimTrackerHitCollection &inputhits);
 
@@ -35,7 +35,7 @@ namespace eicrecon {
     /** algorithm logger */
     std::shared_ptr<spdlog::logger>   m_log;
     const dd4hep::Detector* m_detector{nullptr};
-    std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_cellid_converter = nullptr;
+    const dd4hep::rec::CellIDPositionConverter* m_converter{nullptr};
 
   };
 }
diff --git a/src/algorithms/tracking/ActsGeometryProvider.cc b/src/algorithms/tracking/ActsGeometryProvider.cc
@@ -72,7 +72,7 @@ void draw_surfaces(std::shared_ptr<const Acts::TrackingGeometry> trk_geo, const
 }
 
 
-void ActsGeometryProvider::initialize(dd4hep::Detector *dd4hep_geo,
+void ActsGeometryProvider::initialize(const dd4hep::Detector* dd4hep_geo,
                                       std::string material_file,
                                       std::shared_ptr<spdlog::logger> log,
                                       std::shared_ptr<spdlog::logger> init_log) {

diff --git a/src/algorithms/tracking/ActsGeometryProvider.h b/src/algorithms/tracking/ActsGeometryProvider.h
@@ -32,7 +32,6 @@ namespace dd4hep {
     class Detector;
 }
 namespace dd4hep::rec {
-    class CellIDPositionConverter;
     class Surface;
 }
 
@@ -47,12 +46,12 @@ class ActsGeometryProvider {
     ActsGeometryProvider() {}
     using VolumeSurfaceMap = std::unordered_map<uint64_t, const Acts::Surface *>;
 
-    virtual void initialize(dd4hep::Detector* dd4hep_geo,
+    virtual void initialize(const dd4hep::Detector* dd4hep_geo,
                             std::string material_file,
                             std::shared_ptr<spdlog::logger> log,
                             std::shared_ptr<spdlog::logger> init_log) final;
 
-    dd4hep::Detector*  dd4hepDetector() const {return m_dd4hepDetector; }
+    const dd4hep::Detector* dd4hepDetector() const { return m_dd4hepDetector; }
 
     /** Gets the ACTS tracking geometry.
      */
@@ -85,7 +84,7 @@ class ActsGeometryProvider {
      * This is the main dd4hep detector handle.
      * <a href="https://dd4hep.web.cern.ch/dd4hep/reference/classdd4hep_1_1Detector.html">See DD4hep Detector documentation</a>
      */
-    dd4hep::Detector *m_dd4hepDetector = nullptr;
+    const dd4hep::Detector* m_dd4hepDetector = nullptr;
 
     /// DD4hep surface map
     std::map<int64_t, dd4hep::rec::Surface *> m_surfaceMap;
@@ -105,12 +104,6 @@ class ActsGeometryProvider {
     /// ACTS surface lookup container for hit surfaces that generate smeared hits
     VolumeSurfaceMap m_surfaces;
 
-    /** DD4hep CellID tool.
-     *  Use to lookup geometry information for a hit with cellid number (int64_t).
-     *  <a href="https://dd4hep.web.cern.ch/dd4hep/reference/classdd4hep_1_1rec_1_1CellIDPositionConverter.html">See DD4hep CellIDPositionConverter documentation</a>
-     */
-    std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_cellid_converter = nullptr;
-
     /// Acts magnetic field
     std::shared_ptr<const eicrecon::BField::DD4hepBField> m_magneticField = nullptr;
 

diff --git a/src/algorithms/tracking/DD4hepBField.h b/src/algorithms/tracking/DD4hepBField.h
@@ -8,6 +8,7 @@
 
 #pragma once
 
+#include <gsl/gsl>
 #include <variant>
 
 #include <Acts/Definitions/Algebra.hpp>
@@ -35,7 +36,7 @@ namespace eicrecon::BField {
    */
   class DD4hepBField final : public Acts::MagneticFieldProvider {
   public:
-      dd4hep::Detector* m_det;
+      gsl::not_null<const dd4hep::Detector*> m_det;
 
   public:
     struct Cache {
@@ -51,7 +52,7 @@ namespace eicrecon::BField {
     *
     * @param [in] DD4hep detector instance
     */
-    explicit DD4hepBField(dd4hep::Detector* det) : m_det(det) {}
+    explicit DD4hepBField(gsl::not_null<const dd4hep::Detector*> det) : m_det(det) {}
 
     /**  retrieve magnetic field value.
      *

diff --git a/src/algorithms/tracking/TrackerHitReconstruction.cc b/src/algorithms/tracking/TrackerHitReconstruction.cc
@@ -16,12 +16,11 @@ namespace {
     }
 } // namespace
 
-void TrackerHitReconstruction::init(const dd4hep::Detector* detector, std::shared_ptr<spdlog::logger>& logger) {
+void TrackerHitReconstruction::init(const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger) {
 
     m_log = logger;
 
-    // Create CellID converter
-    m_cellid_converter = std::make_shared<const dd4hep::rec::CellIDPositionConverter>(const_cast<dd4hep::Detector&>(*detector));
+    m_converter = converter;
 }
 
 std::unique_ptr<edm4eic::TrackerHitCollection> TrackerHitReconstruction::process(const edm4eic::RawTrackerHitCollection& raw_hits) {
@@ -34,8 +33,8 @@ std::unique_ptr<edm4eic::TrackerHitCollection> TrackerHitReconstruction::process
         auto id = raw_hit.getCellID();
 
         // Get position and dimension
-        auto pos = m_cellid_converter->position(id);
-        auto dim = m_cellid_converter->cellDimensions(id);
+        auto pos = m_converter->position(id);
+        auto dim = m_converter->cellDimensions(id);
 
         // >oO trace
         if(m_log->level() == spdlog::level::trace) {

diff --git a/src/algorithms/tracking/TrackerHitReconstruction.h b/src/algorithms/tracking/TrackerHitReconstruction.h
@@ -10,7 +10,6 @@
 #include "TrackerHitReconstructionConfig.h"
 #include "algorithms/interfaces/WithPodConfig.h"
 
-#include <DD4hep/Detector.h>
 #include <DDRec/CellIDPositionConverter.h>
 
 namespace eicrecon {
@@ -22,7 +21,7 @@ namespace eicrecon {
 
     public:
         /// Once in a lifetime initialization
-        void init(const dd4hep::Detector *detector, std::shared_ptr<spdlog::logger>& logger);
+        void init(const dd4hep::rec::CellIDPositionConverter* converter, std::shared_ptr<spdlog::logger>& logger);
 
         /// Processes RawTrackerHit and produces a TrackerHit
         std::unique_ptr<edm4eic::TrackerHitCollection> process(const edm4eic::RawTrackerHitCollection& raw_hits);
@@ -35,6 +34,6 @@ namespace eicrecon {
         std::shared_ptr<spdlog::logger> m_log;
 
         /// Cell ID position converter
-        std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_cellid_converter;
+        const dd4hep::rec::CellIDPositionConverter* m_converter;
     };
 }
diff --git a/src/algorithms/tracking/TrackerSourceLinker.cc b/src/algorithms/tracking/TrackerSourceLinker.cc
@@ -24,13 +24,14 @@
 #include <utility>
 
 
-void eicrecon::TrackerSourceLinker::init(std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> cellid_converter,
+void eicrecon::TrackerSourceLinker::init(const dd4hep::Detector* detector,
+                                         const dd4hep::rec::CellIDPositionConverter* converter,
                                          std::shared_ptr<const ActsGeometryProvider> acts_context,
                                          std::shared_ptr<spdlog::logger> logger) {
-    m_cellid_converter = std::move(cellid_converter);
+    m_dd4hepGeo = detector;
+    m_converter = converter;
     m_log = std::move(logger);
     m_acts_context = std::move(acts_context);
-    m_dd4hepGeo = m_acts_context->dd4hepDetector();
     m_detid_b0tracker = m_dd4hepGeo->constant<int>("B0Tracker_Station_1_ID");
 }
 
@@ -57,7 +58,7 @@ eicrecon::TrackerSourceLinkerResult *eicrecon::TrackerSourceLinker::produce(std:
         cov(1, 1) = hit->getPositionError().yy * mm_acts * mm_acts;
 
 
-        const auto* vol_ctx = m_cellid_converter->findContext(hit->getCellID());
+        const auto* vol_ctx = m_converter->findContext(hit->getCellID());
         auto vol_id = vol_ctx->identifier;
 
 

diff --git a/src/algorithms/tracking/TrackerSourceLinker.h b/src/algorithms/tracking/TrackerSourceLinker.h
@@ -21,7 +21,8 @@ namespace eicrecon {
 
     class TrackerSourceLinker {
     public:
-        void init(std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> cellid_converter,
+        void init(const dd4hep::Detector* detector,
+                  const dd4hep::rec::CellIDPositionConverter* converter,
                   std::shared_ptr<const ActsGeometryProvider> acts_context,
                   std::shared_ptr<spdlog::logger> logger);
 
@@ -30,13 +31,12 @@ namespace eicrecon {
     private:
         std::shared_ptr<spdlog::logger> m_log;
 
-        /// Cell ID position converter
-        std::shared_ptr<const dd4hep::rec::CellIDPositionConverter> m_cellid_converter;
+        /// Geometry and Cell ID position converter
+        const dd4hep::Detector* m_dd4hepGeo;
+        const dd4hep::rec::CellIDPositionConverter* m_converter;
 
         std::shared_ptr<const ActsGeometryProvider> m_acts_context;
 
-        dd4hep::Detector* m_dd4hepGeo;
-
         /// Detector-specific information
         int m_detid_b0tracker;
     };

diff --git a/src/benchmarks/reconstruction/femc_studies/femc_studiesProcessor.cc b/src/benchmarks/reconstruction/femc_studies/femc_studiesProcessor.cc
@@ -16,7 +16,6 @@
 #include <spdlog/fmt/ostr.h>
 
 #include <DD4hep/Detector.h>
-#include <DDRec/CellIDPositionConverter.h>
 
 // Include appropriate class headers. e.g.
 #include <edm4hep/SimCalorimeterHitCollection.h>
@@ -207,8 +206,7 @@ void femc_studiesProcessor::Init() {
   }
 
   std::cout << __PRETTY_FUNCTION__ << " " << __LINE__ << std::endl;
-  dd4hep::Detector* detector = dd4hep_service->detector();
-  dd4hep::rec::CellIDPositionConverter cellid_converter(*detector);
+  auto detector = dd4hep_service->detector();
   std::cout << "--------------------------\nID specification:\n";
   try {
     m_decoder = detector->readout("EcalEndcapPHits").idSpec().decoder();

diff --git a/src/benchmarks/reconstruction/lfhcal_studies/lfhcal_studiesProcessor.cc b/src/benchmarks/reconstruction/lfhcal_studies/lfhcal_studiesProcessor.cc
@@ -16,7 +16,6 @@
 #include <spdlog/fmt/ostr.h>
 
 #include <DD4hep/Detector.h>
-#include <DDRec/CellIDPositionConverter.h>
 
 // Include appropriate class headers. e.g.
 #include <edm4hep/SimCalorimeterHitCollection.h>
@@ -243,8 +242,7 @@ void lfhcal_studiesProcessor::Init() {
   }
 
   std::cout << __PRETTY_FUNCTION__ << " " << __LINE__ << std::endl;
-  dd4hep::Detector* detector = dd4hep_service->detector();
-  dd4hep::rec::CellIDPositionConverter cellid_converter(*detector);
+  auto detector = dd4hep_service->detector();
   std::cout << "--------------------------\nID specification:\n";
   try {
     m_decoder = detector->readout("LFHCALHits").idSpec().decoder();