refactor: FPE masking updates & improvements (#2349)

This PR does three things: 1. change fpe mask comment format `issue:1234` -> `#1234` 2. Line ranges now possible in FPE masking This works both with the explicit mask lists / yaml inputs, but also with markers like ```cpp // MARK: fpeMaskBegin(FLTINV, 1, issue:2284) thisMakesAnFLTINV(); // MARK: fpeMaskEnd(FLTINV) ``` 3. Improved FPE mask reporting in interactive running: When launching the Sequencer, the masks will now be checked if their file paths are valid, and it will show the masked pieces of code.
acts-project · Aug 29, 2023 · e529c52 · e529c52
1 parent d72def3
commit e529c52
Show file tree

Hide file tree

Showing 15 changed files with 277 additions and 71 deletions.
diff --git a/CI/check_fpe_masks.py b/CI/check_fpe_masks.py
@@ -46,7 +46,8 @@ async def check(token: str, repo: str):
                             first = False
                             exp = m.group(1)
                             for m in re.findall(
-                                r"fpeMask\((\w+), ?(\d+) ?, ?issue: ?(\d+)\)", exp
+                                r"fpeMask(?:Begin)?\( ?(\w+), ?(\d+) ?, ?#(\d+) ?\)",
+                                exp,
                             ):
                                 fpeType, count, number = m
 

diff --git a/CI/physmon/fpe_masks.yml b/CI/physmon/fpe_masks.yml
@@ -4,8 +4,6 @@
   FLTUND: 1
 "Core/include/Acts/TrackFitting/detail/GsfUtils.hpp:197":
   FLTUND: 1  
-"Acts/Utilities/GaussianMixtureReduction.hpp:198":
-  FLTUND: 1
 "Core/src/Utilities/AnnealingUtility.cpp:43":
   FLTUND: 1
 "Core/src/Utilities/AnnealingUtility.cpp:40":

diff --git a/CI/physmon/workflows/physmon_track_finding_ttbar.py b/CI/physmon/workflows/physmon_track_finding_ttbar.py
@@ -38,32 +38,38 @@
         Path(__file__).parent.parent / "fpe_masks.yml"
     ) + [
         acts.examples.Sequencer.FpeMask(
-            "Examples/Algorithms/Fatras/src/FatrasSimulation.cpp:172",
+            "Examples/Algorithms/Fatras/src/FatrasSimulation.cpp",
+            (172, 173),
             acts.FpeType.FLTINV,
             1,
         ),
         acts.examples.Sequencer.FpeMask(
-            "Examples/Algorithms/Fatras/src/FatrasSimulation.cpp:172",
+            "Examples/Algorithms/Fatras/src/FatrasSimulation.cpp",
+            (172, 173),
             acts.FpeType.FLTOVF,
             1,
         ),
         acts.examples.Sequencer.FpeMask(
-            "Examples/Io/Root/src/RootTrajectorySummaryWriter.cpp:371",
+            "Examples/Io/Root/src/RootTrajectorySummaryWriter.cpp",
+            (371, 372),
             acts.FpeType.FLTINV,
             1,
         ),
         acts.examples.Sequencer.FpeMask(
-            "Core/src/Utilities/AnnealingUtility.cpp:38",
+            "Core/src/Utilities/AnnealingUtility.cpp",
+            (38, 39),
             acts.FpeType.FLTUND,
             1,
         ),
         acts.examples.Sequencer.FpeMask(
-            "Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp:110",
+            "Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp",
+            (110, 111),
             acts.FpeType.FLTINV,
             1,
         ),
         acts.examples.Sequencer.FpeMask(
-            "Fatras/include/ActsFatras/Kernel/Simulation.hpp:98",
+            "Fatras/include/ActsFatras/Kernel/Simulation.hpp",
+            (98, 99),
             acts.FpeType.FLTOVF,
             1,
         ),

diff --git a/Core/include/Acts/Utilities/GaussianMixtureReduction.hpp b/Core/include/Acts/Utilities/GaussianMixtureReduction.hpp
@@ -85,7 +85,7 @@ auto gaussianMixtureCov(const components_t components,
   for (const auto &cmp : components) {
     const auto &[weight_l, pars_l, cov_l] = projector(cmp);
 
-    cov += weight_l * cov_l;  // MARK: fpeMask(FLTUND, 1, issue:2347)
+    cov += weight_l * cov_l;  // MARK: fpeMask(FLTUND, 1, #2347)
 
     ActsVector<D> diff = pars_l - mean;
 
@@ -195,8 +195,10 @@ auto gaussianMixtureMeanCov(const components_t components,
 
   std::apply([&](auto... dsc) { (wrap(dsc), ...); }, angleDesc);
 
+  // MARK: fpeMaskBegin(FLTUND, 1, #2347)
   const auto cov =
       gaussianMixtureCov(components, mean, sumOfWeights, projector, angleDesc);
+  // MARK: fpeMaskEnd(FLTUND)
 
   return RetType{mean, cov};
 }

diff --git a/Examples/Framework/include/ActsExamples/Framework/Sequencer.hpp b/Examples/Framework/include/ActsExamples/Framework/Sequencer.hpp
@@ -58,7 +58,8 @@ class SequenceConfigurationException : public std::runtime_error {
 class Sequencer {
  public:
   struct FpeMask {
-    std::string loc;
+    std::string file;
+    std::pair<std::size_t, std::size_t> lines;
     Acts::FpeType type;
     std::size_t count;
   };
@@ -186,9 +187,7 @@ class Sequencer {
   const Acts::Logger &logger() const { return *m_logger; }
 };
 
-inline std::ostream &operator<<(std::ostream &os, const Sequencer::FpeMask &m) {
-  os << "FpeMask(" << m.loc << ", " << m.type << " <= " << m.count << ")";
-  return os;
-}
+std::ostream &operator<<(std::ostream &os,
+                         const ActsExamples::Sequencer::FpeMask &m);
 
 }  // namespace ActsExamples
diff --git a/Examples/Framework/src/Framework/Sequencer.cpp b/Examples/Framework/src/Framework/Sequencer.cpp
@@ -37,6 +37,7 @@
 #include <ratio>
 #include <regex>
 #include <stdexcept>
+#include <string>
 #include <string_view>
 #include <typeinfo>
 
@@ -655,9 +656,17 @@ std::pair<std::string, std::size_t> Sequencer::fpeMaskCount(
     const boost::stacktrace::stacktrace& st, Acts::FpeType type) const {
   for (const auto& frame : st) {
     std::string loc = Acts::FpeMonitor::getSourceLocation(frame);
-    for (const auto& [filt, fType, count] : m_cfg.fpeMasks) {
-      if (boost::algorithm::ends_with(loc, filt) && fType == type) {
-        return {filt, count};
+    auto it = loc.find_last_of(':');
+    std::string locFile = loc.substr(0, it);
+    unsigned int locLine = std::stoi(loc.substr(it + 1));
+    for (const auto& [file, lines, fType, count] : m_cfg.fpeMasks) {
+      const auto [start, end] = lines;
+      if (boost::algorithm::ends_with(locFile, file) &&
+          (start <= locLine && locLine < end) && fType == type) {
+        std::string ls = start + 1 == end ? std::to_string(start)
+                                          : "(" + std::to_string(start) + ", " +
+                                                std::to_string(end) + "]";
+        return {file + ":" + ls, count};
       }
     }
   }
@@ -674,4 +683,17 @@ Acts::FpeMonitor::Result Sequencer::fpeResult() const {
   return merged;
 }
 
+std::ostream& operator<<(std::ostream& os,
+                         const ActsExamples::Sequencer::FpeMask& m) {
+  os << "FpeMask(" << m.file << ":";
+
+  if (m.lines.first + 1 == m.lines.second) {
+    os << m.lines.first;
+  } else {
+    os << "(" << m.lines.first << ", " << m.lines.second << "]";
+  }
+  os << ", " << m.type << " <= " << m.count << ")";
+  return os;
+}
+
 }  // namespace ActsExamples
diff --git a/Examples/Io/Root/src/RootTrajectoryStatesWriter.cpp b/Examples/Io/Root/src/RootTrajectoryStatesWriter.cpp
@@ -543,10 +543,10 @@ ActsExamples::ProcessCode ActsExamples::RootTrajectoryStatesWriter::writeT(
 
             // track parameters error
             m_err_eLOC0[ipar].push_back(
-                std::sqrt(covariance(  // MARK: fpeMask(FLTINV, 1, issue:2348)
+                std::sqrt(covariance(  // MARK: fpeMask(FLTINV, 1, #2348)
                     Acts::eBoundLoc0, Acts::eBoundLoc0)));
             m_err_eLOC1[ipar].push_back(
-                std::sqrt(covariance(  // MARK: fpeMask(FLTINV, 1, issue:2348)
+                std::sqrt(covariance(  // MARK: fpeMask(FLTINV, 1, #2348)
                     Acts::eBoundLoc1, Acts::eBoundLoc1)));
             m_err_ePHI[ipar].push_back(
                 std::sqrt(covariance(Acts::eBoundPhi, Acts::eBoundPhi)));

diff --git a/Examples/Io/Root/src/RootTrajectorySummaryWriter.cpp b/Examples/Io/Root/src/RootTrajectorySummaryWriter.cpp
@@ -368,8 +368,7 @@ ActsExamples::ProcessCode ActsExamples::RootTrajectorySummaryWriter::writeT(
 
         if (hasFittedCov) {
           for (unsigned int i = 0; i < Acts::eBoundSize; ++i) {
-            pull[i] =
-                res[i] / error[i];  // MARK: fpeMask(FLTINV, 1, issue: 2284)
+            pull[i] = res[i] / error[i];  // MARK: fpeMask(FLTINV, 1, #2284)
           }
         }
       }