fix: Fix AMVF finding with negative cov (#2374)

AMVF vertex finding dropped vertex candidates because `isMergedVertex` would return `true` for negative covariance matrices. The reason why these matrices are negative definite is still unclear but we should not consider them as merged vertices by default I think. This was the reason why we experienced noisy AMVF physmon diff for mostly unrelated changes because these conditions would flip flop on small numerical changes. A small reproducible example can be found in #2372
acts-project · Aug 17, 2023 · 1c459a9 · 1c459a9
1 parent 543bd5a
commit 1c459a9
Show file tree

Hide file tree

Showing 6 changed files with 10 additions and 9 deletions.
diff --git a/CI/physmon/reference/performance_amvf_orthogonal_hist.root b/CI/physmon/reference/performance_amvf_orthogonal_hist.root
diff --git a/CI/physmon/reference/performance_amvf_seeded_hist.root b/CI/physmon/reference/performance_amvf_seeded_hist.root
diff --git a/CI/physmon/reference/performance_amvf_truth_estimated_hist.root b/CI/physmon/reference/performance_amvf_truth_estimated_hist.root
diff --git a/CI/physmon/reference/performance_amvf_truth_smeared_hist.root b/CI/physmon/reference/performance_amvf_truth_smeared_hist.root
diff --git a/CI/physmon/reference/performance_amvf_ttbar_hist.root b/CI/physmon/reference/performance_amvf_ttbar_hist.root
diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp
@@ -522,20 +522,21 @@ auto Acts::AdaptiveMultiVertexFinder<vfitter_t, sfinder_t>::isMergedVertex(
 
     double significance = 0;
     if (not m_cfg.do3dSplitting) {
-      // Use only z significance
-      if (sumCovZ > 0.) {
-        significance = std::abs(deltaZPos) / std::sqrt(sumCovZ);
-      } else {
-        return true;
+      if (sumCovZ <= 0) {
+        // TODO FIXME this should never happen
+        continue;
       }
+      // Use only z significance
+      significance = std::abs(deltaZPos) / std::sqrt(sumCovZ);
     } else {
       // Use full 3d information for significance
       SymMatrix4 sumCov = candidateCov + otherCov;
-      if (auto sumCovInverse = safeInverse(sumCov); sumCovInverse) {
-        significance = std::sqrt(deltaPos.dot(*sumCovInverse * deltaPos));
-      } else {
-        return true;
+      auto sumCovInverse = safeInverse(sumCov);
+      if (!sumCovInverse) {
+        // TODO FIXME this should never happen
+        continue;
       }
+      significance = std::sqrt(deltaPos.dot(*sumCovInverse * deltaPos));
     }
     if (significance < m_cfg.maxMergeVertexSignificance) {
       return true;