refactor!: Using non const InternalSpacePoint objects (#1196)

This PR tries to fix usage of mutable properties in #1168 and #1143. Tagging: @paulgessinger @robertlangenberg @LuisFelipeCoelho BREAKING CHANGE: Remove assumption on constness of InternalSpacePoint. The need of this major change is to allow seed finding and filtering to evaluate and set properties of InternalSpacePoints as soon as they are candidates for triplet formation and seed quality evaluation.
acts-project · Mar 18, 2022 · be5fec4 · be5fec4
1 parent 1674bce
commit be5fec4
Show file tree

Hide file tree

Showing 13 changed files with 88 additions and 84 deletions.
diff --git a/Core/include/Acts/Seeding/BinnedSPGroup.hpp b/Core/include/Acts/Seeding/BinnedSPGroup.hpp
@@ -30,7 +30,7 @@ template <typename external_spacepoint_t>
 class NeighborhoodIterator {
  public:
   using sp_it_t = typename std::vector<std::unique_ptr<
-      const InternalSpacePoint<external_spacepoint_t>>>::const_iterator;
+      InternalSpacePoint<external_spacepoint_t>>>::const_iterator;
 
   NeighborhoodIterator() = delete;
 
@@ -94,7 +94,7 @@ class NeighborhoodIterator {
     }
   }
 
-  const InternalSpacePoint<external_spacepoint_t>* operator*() {
+  InternalSpacePoint<external_spacepoint_t>* operator*() {
     return (*m_curIt).get();
   }
 

diff --git a/Core/include/Acts/Seeding/BinnedSPGroup.ipp b/Core/include/Acts/Seeding/BinnedSPGroup.ipp
@@ -35,8 +35,8 @@ Acts::BinnedSPGroup<external_spacepoint_t>::BinnedSPGroup(
   // create number of bins equal to number of millimeters rMax
   // (worst case minR: configured minR + 1mm)
   size_t numRBins = (config.rMax + config.beamPos.norm());
-  std::vector<std::vector<
-      std::unique_ptr<const InternalSpacePoint<external_spacepoint_t>>>>
+  std::vector<
+      std::vector<std::unique_ptr<InternalSpacePoint<external_spacepoint_t>>>>
       rBins(numRBins);
   for (spacepoint_iterator_t it = spBegin; it != spEnd; it++) {
     if (*it == nullptr) {
@@ -58,9 +58,8 @@ Acts::BinnedSPGroup<external_spacepoint_t>::BinnedSPGroup(
       continue;
     }
 
-    auto isp =
-        std::make_unique<const InternalSpacePoint<external_spacepoint_t>>(
-            sp, spPosition, config.beamPos, variance);
+    auto isp = std::make_unique<InternalSpacePoint<external_spacepoint_t>>(
+        sp, spPosition, config.beamPos, variance);
     // calculate r-Bin index and protect against overflow (underflow not
     // possible)
     size_t rIndex = isp->radius();
@@ -75,8 +74,7 @@ Acts::BinnedSPGroup<external_spacepoint_t>::BinnedSPGroup(
   for (auto& rbin : rBins) {
     for (auto& isp : rbin) {
       Acts::Vector2 spLocation(isp->phi(), isp->z());
-      std::vector<
-          std::unique_ptr<const InternalSpacePoint<external_spacepoint_t>>>&
+      std::vector<std::unique_ptr<InternalSpacePoint<external_spacepoint_t>>>&
           bin = grid->atPosition(spLocation);
       bin.push_back(std::move(isp));
     }

diff --git a/Core/include/Acts/Seeding/InternalSeed.hpp b/Core/include/Acts/Seeding/InternalSeed.hpp
@@ -21,12 +21,12 @@ class InternalSeed {
   /////////////////////////////////////////////////////////////////////////////////
 
  public:
-  InternalSeed(const InternalSpacePoint<SpacePoint>& s0,
-               const InternalSpacePoint<SpacePoint>& s1,
-               const InternalSpacePoint<SpacePoint>& s2, float z);
+  InternalSeed(InternalSpacePoint<SpacePoint>& s0,
+               InternalSpacePoint<SpacePoint>& s1,
+               InternalSpacePoint<SpacePoint>& s2, float z);
   InternalSeed& operator=(const InternalSeed& seed);
 
-  const std::array<const InternalSpacePoint<SpacePoint>*, 3> sp;
+  const std::array<InternalSpacePoint<SpacePoint>*, 3> sp;
   float z() const { return m_z; }
 
  protected:
@@ -49,9 +49,8 @@ inline InternalSeed<SpacePoint>& InternalSeed<SpacePoint>::operator=(
 
 template <typename SpacePoint>
 inline InternalSeed<SpacePoint>::InternalSeed(
-    const InternalSpacePoint<SpacePoint>& s0,
-    const InternalSpacePoint<SpacePoint>& s1,
-    const InternalSpacePoint<SpacePoint>& s2, float z)
+    InternalSpacePoint<SpacePoint>& s0, InternalSpacePoint<SpacePoint>& s1,
+    InternalSpacePoint<SpacePoint>& s2, float z)
     : sp({&s0, &s1, &s2}) {
   m_z = z;
 }

diff --git a/Core/include/Acts/Seeding/InternalSpacePoint.hpp b/Core/include/Acts/Seeding/InternalSpacePoint.hpp
@@ -40,16 +40,32 @@ class InternalSpacePoint {
   float phi() const { return atan2f(m_y, m_x); }
   const float& varianceR() const { return m_varianceR; }
   const float& varianceZ() const { return m_varianceZ; }
+  const float& quality() const { return m_quality; }
+  const float& cotTheta() const { return m_cotTheta; }
+  void setCotTheta(float cotTheta) { m_cotTheta = cotTheta; }
+  void setQuality(float quality) {
+    if (quality >= m_quality) {
+      m_quality = quality;
+    }
+  }
   const SpacePoint& sp() const { return m_sp; }
 
  protected:
-  float m_x;               // x-coordinate in beam system coordinates
-  float m_y;               // y-coordinate in beam system coordinates
-  float m_z;               // z-coordinate in beam system coordinetes
-  float m_r;               // radius       in beam system coordinates
-  float m_varianceR;       //
-  float m_varianceZ;       //
-  const SpacePoint& m_sp;  // external space point
+  float m_x;          // x-coordinate in beam system coordinates
+  float m_y;          // y-coordinate in beam system coordinates
+  float m_z;          // z-coordinate in beam system coordinetes
+  float m_r;          // radius       in beam system coordinates
+  float m_varianceR;  //
+  float m_varianceZ;  //
+  float m_cotTheta = std::numeric_limits<
+      double>::quiet_NaN();  // 1/tanTheta estimated from central+this space
+                             // point. Its evaluation requires that the space
+                             // point is a candidate for triplet search.
+  float m_quality = -std::numeric_limits<
+      double>::infinity();  // Quality score of the seed the space point is used
+                            // for. Quality can be changed if the space point is
+                            // used for a better quality seed.
+  const SpacePoint& m_sp;   // external space point
 };
 
 /////////////////////////////////////////////////////////////////////////////////
@@ -75,14 +91,6 @@ inline InternalSpacePoint<SpacePoint>::InternalSpacePoint(
 
 template <typename SpacePoint>
 inline InternalSpacePoint<SpacePoint>::InternalSpacePoint(
-    const InternalSpacePoint<SpacePoint>& sp)
-    : m_sp(sp.sp()) {
-  m_x = sp.m_x;
-  m_y = sp.m_y;
-  m_z = sp.m_z;
-  m_r = sp.m_r;
-  m_varianceR = sp.m_varianceR;
-  m_varianceZ = sp.m_varianceZ;
-}
+    const InternalSpacePoint<SpacePoint>& sp) = default;
 
 }  // end of namespace Acts
diff --git a/Core/include/Acts/Seeding/SeedFilter.hpp b/Core/include/Acts/Seeding/SeedFilter.hpp
@@ -42,9 +42,9 @@ class SeedFilter {
   /// @param zOrigin on the z axis as defined by bottom and middle space point
   /// @param outIt Output iterator for the seeds
   virtual void filterSeeds_2SpFixed(
-      const InternalSpacePoint<external_spacepoint_t>& bottomSP,
-      const InternalSpacePoint<external_spacepoint_t>& middleSP,
-      std::vector<const InternalSpacePoint<external_spacepoint_t>*>& topSpVec,
+      InternalSpacePoint<external_spacepoint_t>& bottomSP,
+      InternalSpacePoint<external_spacepoint_t>& middleSP,
+      std::vector<InternalSpacePoint<external_spacepoint_t>*>& topSpVec,
       std::vector<float>& invHelixDiameterVec,
       std::vector<float>& impactParametersVec, float zOrigin,
       std::back_insert_iterator<std::vector<std::pair<

diff --git a/Core/include/Acts/Seeding/SeedFilter.ipp b/Core/include/Acts/Seeding/SeedFilter.ipp
@@ -21,9 +21,9 @@ SeedFilter<external_spacepoint_t>::SeedFilter(
 // return vector must contain weight of each seed
 template <typename external_spacepoint_t>
 void SeedFilter<external_spacepoint_t>::filterSeeds_2SpFixed(
-    const InternalSpacePoint<external_spacepoint_t>& bottomSP,
-    const InternalSpacePoint<external_spacepoint_t>& middleSP,
-    std::vector<const InternalSpacePoint<external_spacepoint_t>*>& topSpVec,
+    InternalSpacePoint<external_spacepoint_t>& bottomSP,
+    InternalSpacePoint<external_spacepoint_t>& middleSP,
+    std::vector<InternalSpacePoint<external_spacepoint_t>*>& topSpVec,
     std::vector<float>& invHelixDiameterVec,
     std::vector<float>& impactParametersVec, float zOrigin,
     std::back_insert_iterator<std::vector<std::pair<
@@ -141,6 +141,12 @@ void SeedFilter<external_spacepoint_t>::filterSeeds_1SpFixed(
   // ordering by weight by filterSeeds_2SpFixed means these are the lowest
   // weight seeds
   for (; it < itBegin + maxSeeds; ++it) {
+    float bestSeedQuality = (*it).first;
+
+    (*it).second->sp[0]->setQuality(bestSeedQuality);
+    (*it).second->sp[1]->setQuality(bestSeedQuality);
+    (*it).second->sp[2]->setQuality(bestSeedQuality);
+
     outIt = Seed<external_spacepoint_t>{
         (*it).second->sp[0]->sp(), (*it).second->sp[1]->sp(),
         (*it).second->sp[2]->sp(), (*it).second->z()};

diff --git a/Core/include/Acts/Seeding/SeedFinderUtils.hpp b/Core/include/Acts/Seeding/SeedFinderUtils.hpp
@@ -32,9 +32,9 @@ struct LinCircle {
 /// @param[in] spM The middle spacepoint to use.
 /// @param[in] bottom Should be true if sp is a bottom SP.
 template <typename external_spacepoint_t>
-LinCircle transformCoordinates(
-    const InternalSpacePoint<external_spacepoint_t>& sp,
-    const InternalSpacePoint<external_spacepoint_t>& spM, bool bottom);
+LinCircle transformCoordinates(InternalSpacePoint<external_spacepoint_t>& sp,
+                               InternalSpacePoint<external_spacepoint_t>& spM,
+                               bool bottom);
 
 /// @brief Transform a vector of spacepoints to u-v space circles with respect
 /// to a given middle spacepoint.
@@ -48,8 +48,8 @@ LinCircle transformCoordinates(
 /// @param[out] linCircleVec The output vector to write to.
 template <typename external_spacepoint_t>
 void transformCoordinates(
-    const std::vector<const InternalSpacePoint<external_spacepoint_t>*>& vec,
-    const InternalSpacePoint<external_spacepoint_t>& spM, bool bottom,
+    const std::vector<InternalSpacePoint<external_spacepoint_t>*>& vec,
+    InternalSpacePoint<external_spacepoint_t>& spM, bool bottom,
     bool enableCutsForSortedSP, std::vector<LinCircle>& linCircleVec);
 }  // namespace Acts
 

diff --git a/Core/include/Acts/Seeding/SeedFinderUtils.ipp b/Core/include/Acts/Seeding/SeedFinderUtils.ipp
@@ -8,9 +8,9 @@
 
 namespace Acts {
 template <typename external_spacepoint_t>
-LinCircle transformCoordinates(
-    const InternalSpacePoint<external_spacepoint_t>& sp,
-    const InternalSpacePoint<external_spacepoint_t>& spM, bool bottom) {
+LinCircle transformCoordinates(InternalSpacePoint<external_spacepoint_t>& sp,
+                               InternalSpacePoint<external_spacepoint_t>& spM,
+                               bool bottom) {
   // The computation inside this function is exactly identical to that in the
   // vectorized version of this function, except that it operates on a single
   // spacepoint. Please see the other version of this function for more
@@ -46,8 +46,8 @@ LinCircle transformCoordinates(
 
 template <typename external_spacepoint_t>
 void transformCoordinates(
-    const std::vector<const InternalSpacePoint<external_spacepoint_t>*>& vec,
-    const InternalSpacePoint<external_spacepoint_t>& spM, bool bottom,
+    const std::vector<InternalSpacePoint<external_spacepoint_t>*>& vec,
+    InternalSpacePoint<external_spacepoint_t>& spM, bool bottom,
     bool enableCutsForSortedSP, std::vector<LinCircle>& linCircleVec) {
   float xM = spM.x();
   float yM = spM.y();
@@ -93,6 +93,7 @@ void transformCoordinates(
             (cot_theta * cot_theta) * (varianceRM + sp->varianceR())) *
            iDeltaR2;
     linCircleVec.push_back(l);
+    sp->setCotTheta(cot_theta);
   }
   // sort the SP in order of cotTheta
   if (enableCutsForSortedSP) {

diff --git a/Core/include/Acts/Seeding/Seedfinder.hpp b/Core/include/Acts/Seeding/Seedfinder.hpp
@@ -34,17 +34,16 @@ class Seedfinder {
  public:
   struct State {
     // bottom space point
-    std::vector<const InternalSpacePoint<external_spacepoint_t>*>
-        compatBottomSP;
-    std::vector<const InternalSpacePoint<external_spacepoint_t>*> compatTopSP;
+    std::vector<InternalSpacePoint<external_spacepoint_t>*> compatBottomSP;
+    std::vector<InternalSpacePoint<external_spacepoint_t>*> compatTopSP;
     // contains parameters required to calculate circle with linear equation
     // ...for bottom-middle
     std::vector<LinCircle> linCircleBottom;
     // ...for middle-top
     std::vector<LinCircle> linCircleTop;
 
     // create vectors here to avoid reallocation in each loop
-    std::vector<const InternalSpacePoint<external_spacepoint_t>*> topSpVec;
+    std::vector<InternalSpacePoint<external_spacepoint_t>*> topSpVec;
     std::vector<float> curvatures;
     std::vector<float> impactParameters;
     std::vector<float> etaVec;

diff --git a/Core/include/Acts/Seeding/SpacePointGrid.hpp b/Core/include/Acts/Seeding/SpacePointGrid.hpp
@@ -59,8 +59,7 @@ struct SpacePointGridConfig {
 
 template <typename external_spacepoint_t>
 using SpacePointGrid = detail::Grid<
-    std::vector<
-        std::unique_ptr<const InternalSpacePoint<external_spacepoint_t>>>,
+    std::vector<std::unique_ptr<InternalSpacePoint<external_spacepoint_t>>>,
     detail::Axis<detail::AxisType::Equidistant,
                  detail::AxisBoundaryType::Closed>,
     detail::Axis<detail::AxisType::Variable, detail::AxisBoundaryType::Bound>>;

diff --git a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp
@@ -66,11 +66,9 @@ Seedfinder<external_spacepoint_t, Acts::Cuda>::createSeedsForGroup(
   // Algorithm 0. Matrix Flattening
   //---------------------------------
 
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      middleSPvec;
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      bottomSPvec;
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*> topSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> middleSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> bottomSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> topSPvec;
 
   // Get the size of spacepoints
   int nSpM(0);
@@ -296,4 +294,4 @@ Seedfinder<external_spacepoint_t, Acts::Cuda>::createSeedsForGroup(
   ACTS_CUDA_ERROR_CHECK(cudaStreamDestroy(cuStream));
   return outputVec;
 }
-}  // namespace Acts
+}  // namespace Acts
diff --git a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp
@@ -81,18 +81,18 @@ SeedFinder<external_spacepoint_t>::createSeedsForGroup(
 
   // Create more convenient vectors out of the space point containers.
   auto spVecMaker = [](sp_range_t spRange) {
-    std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*> result;
+    std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> result;
     for (auto* sp : spRange) {
       result.push_back(sp);
     }
     return result;
   };
 
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      bottomSPVec(spVecMaker(bottomSPs));
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      middleSPVec(spVecMaker(middleSPs));
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*> topSPVec(
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> bottomSPVec(
+      spVecMaker(bottomSPs));
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> middleSPVec(
+      spVecMaker(middleSPs));
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> topSPVec(
       spVecMaker(topSPs));
 
   // If either one of them is empty, we have nothing to find.
@@ -193,12 +193,12 @@ SeedFinder<external_spacepoint_t>::createSeedsForGroup(
     std::vector<std::pair<
         float, std::unique_ptr<const InternalSeed<external_spacepoint_t>>>>
         seedsPerSPM;
-    const auto& middleSP = *(middleSPVec[middleIndex]);
+    auto& middleSP = *(middleSPVec[middleIndex]);
     for (const Details::Triplet& triplet : *triplet_itr) {
       assert(triplet.bottomIndex < bottomSPVec.size());
-      const auto& bottomSP = *(bottomSPVec[triplet.bottomIndex]);
+      auto& bottomSP = *(bottomSPVec[triplet.bottomIndex]);
       assert(triplet.topIndex < topSPVec.size());
-      const auto& topSP = *(topSPVec[triplet.topIndex]);
+      auto& topSP = *(topSPVec[triplet.topIndex]);
       seedsPerSPM.emplace_back(std::make_pair(
           triplet.weight,
           std::make_unique<const InternalSeed<external_spacepoint_t>>(
@@ -222,4 +222,4 @@ void SeedFinder<external_spacepoint_t>::setLogger(
 }
 
 }  // namespace Cuda
-}  // namespace Acts
+}  // namespace Acts
diff --git a/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp b/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp
@@ -84,37 +84,33 @@ Seedfinder<external_spacepoint_t>::createSeedsForGroup(
   vecmem::vector<detail::DeviceSpacePoint> deviceMiddleSPs(m_resource);
   vecmem::vector<detail::DeviceSpacePoint> deviceTopSPs(m_resource);
 
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      bottomSPvec;
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*>
-      middleSPvec;
-  std::vector<const Acts::InternalSpacePoint<external_spacepoint_t>*> topSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> bottomSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> middleSPvec;
+  std::vector<Acts::InternalSpacePoint<external_spacepoint_t>*> topSPvec;
 
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP : bottomSPs) {
+  for (auto SP : bottomSPs) {
     bottomSPvec.push_back(SP);
   }
   deviceBottomSPs.reserve(bottomSPvec.size());
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP :
-       bottomSPvec) {
+  for (auto SP : bottomSPvec) {
     deviceBottomSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(),
                                SP->varianceR(), SP->varianceZ()});
   }
 
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP : middleSPs) {
+  for (auto SP : middleSPs) {
     middleSPvec.push_back(SP);
   }
   deviceMiddleSPs.reserve(middleSPvec.size());
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP :
-       middleSPvec) {
+  for (auto SP : middleSPvec) {
     deviceMiddleSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(),
                                SP->varianceR(), SP->varianceZ()});
   }
 
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP : topSPs) {
+  for (auto SP : topSPs) {
     topSPvec.push_back(SP);
   }
   deviceTopSPs.reserve(topSPvec.size());
-  for (const Acts::InternalSpacePoint<external_spacepoint_t>* SP : topSPvec) {
+  for (auto SP : topSPvec) {
     deviceTopSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(),
                             SP->varianceR(), SP->varianceZ()});
   }
@@ -149,4 +145,4 @@ Seedfinder<external_spacepoint_t>::createSeedsForGroup(
   }
   return outputVec;
 }
-}  // namespace Acts::Sycl
+}  // namespace Acts::Sycl