From 5ffd44a39a4c6d65f4df35d08077ee2cdfcb59de Mon Sep 17 00:00:00 2001 From: russofel Date: Mon, 13 Nov 2023 15:06:33 +0100 Subject: [PATCH 1/6] fix --- .../Vertexing/AdaptiveMultiVertexFinder.ipp | 5 +- .../Vertexing/AdaptiveMultiVertexFitter.hpp | 48 +++++++++---------- .../Vertexing/AdaptiveMultiVertexFitter.ipp | 23 +-------- .../AdaptiveMultiVertexFitterTests.cpp | 7 ++- 4 files changed, 30 insertions(+), 53 deletions(-) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp index 243a8879002..1b6239774b2 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp @@ -560,6 +560,7 @@ auto Acts::AdaptiveMultiVertexFinder::deleteLastVertex( // Update fitter state with removed vertex candidate fitterState.removeVertexFromMultiMap(vtx); + fitterState.removeVertexFromCollection(vtx); for (auto& entry : fitterState.tracksAtVerticesMap) { // Delete all linearized tracks for current (bad) vertex @@ -569,8 +570,8 @@ auto Acts::AdaptiveMultiVertexFinder::deleteLastVertex( } // Do the fit with removed vertex - auto fitResult = m_cfg.vertexFitter.addVtxToFit( - fitterState, vtx, m_cfg.linearizer, vertexingOptions); + auto fitResult = + m_cfg.vertexFitter.fit(fitterState, m_cfg.linearizer, vertexingOptions); if (!fitResult.ok()) { return fitResult.error(); } diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp index e26b1e73ed2..76f4cc976bf 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp @@ -98,6 +98,16 @@ class AdaptiveMultiVertexFitter { } } } + + void removeVertexFromCollection(Vertex& vtxToRemove) { + auto it = std::find(vertexCollection.begin(), vertexCollection.end(), + &vtxToRemove); + // Check if the value was found before erasing + if (it != vertexCollection.end()) { + // Erase the element if found + vertexCollection.erase(it); + } + } }; struct Config { @@ -170,20 +180,6 @@ class AdaptiveMultiVertexFitter { m_extractParameters(func), m_logger(std::move(logger)) {} - /// @brief Performs a simultaneous fit of all vertices in `verticesToFit` - /// by invoking `fitImpl`. - /// - /// @param state Fitter state - /// @param verticesToFit Vector containing all vertices to be fitted - /// @param linearizer Track linearizer - /// @param vertexingOptions Vertexing options - /// - /// @return Result object - Result fit( - State& state, const std::vector*>& verticesToFit, - const Linearizer_t& linearizer, - const VertexingOptions& vertexingOptions) const; - /// @brief Adds a new vertex to an existing multi-vertex fit. /// 1. The 3D impact parameters are calculated for all tracks associated /// with newVertex. @@ -204,6 +200,18 @@ class AdaptiveMultiVertexFitter { const Linearizer_t& linearizer, const VertexingOptions& vertexingOptions) const; + /// @brief Performs a simultaneous fit of all vertices in + /// state.vertexCollection + /// + /// @param state Fitter state + /// @param linearizer Track linearizer + /// @param vertexingOptions Vertexing options + /// + /// @return Result object + Result fit( + State& state, const Linearizer_t& linearizer, + const VertexingOptions& vertexingOptions) const; + private: /// Configuration object const Config m_cfg; @@ -219,18 +227,6 @@ class AdaptiveMultiVertexFitter { /// Private access to logging instance const Logger& logger() const { return *m_logger; } - /// @brief Performs a simultaneous fit of all vertices in - /// state.vertexCollection - /// - /// @param state Fitter state - /// @param linearizer Track linearizer - /// @param vertexingOptions Vertexing options - /// - /// @return Result object - Result fitImpl( - State& state, const Linearizer_t& linearizer, - const VertexingOptions& vertexingOptions) const; - /// @brief Tests if vertex is already in list of vertices or not /// /// @param vtx Vertex to test diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp index c4e1de2450d..b7d23ce6c44 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp @@ -13,25 +13,6 @@ template Acts::Result Acts::AdaptiveMultiVertexFitter::fit( - State& state, const std::vector*>& verticesToFit, - const linearizer_t& linearizer, - const VertexingOptions& vertexingOptions) const { - // Set all vertices to fit in the current state - state.vertexCollection = verticesToFit; - - // Perform fit on all vertices simultaneously - auto fitRes = fitImpl(state, linearizer, vertexingOptions); - - if (!fitRes.ok()) { - return fitRes.error(); - } - - return {}; -} - -template -Acts::Result -Acts::AdaptiveMultiVertexFitter::fitImpl( State& state, const linearizer_t& linearizer, const VertexingOptions& vertexingOptions) const { // Reset annealing tool @@ -119,7 +100,7 @@ Acts::AdaptiveMultiVertexFitter::addVtxToFit( return VertexingError::EmptyInput; } - std::vector*> verticesToFit; + std::vector*> verticesToFit = {&newVertex}; // List of vertices added in last iteration std::vector*> lastIterAddedVertices = {&newVertex}; @@ -164,7 +145,7 @@ Acts::AdaptiveMultiVertexFitter::addVtxToFit( state.vertexCollection = verticesToFit; // Perform fit on all added vertices - auto fitRes = fitImpl(state, linearizer, vertexingOptions); + auto fitRes = fit(state, linearizer, vertexingOptions); if (!fitRes.ok()) { return fitRes.error(); } diff --git a/Tests/UnitTests/Core/Vertexing/AdaptiveMultiVertexFitterTests.cpp b/Tests/UnitTests/Core/Vertexing/AdaptiveMultiVertexFitterTests.cpp index ba2c74d299f..f1b96f0416f 100644 --- a/Tests/UnitTests/Core/Vertexing/AdaptiveMultiVertexFitterTests.cpp +++ b/Tests/UnitTests/Core/Vertexing/AdaptiveMultiVertexFitterTests.cpp @@ -442,7 +442,6 @@ BOOST_AUTO_TEST_CASE(adaptive_multi_vertex_fitter_test_athena) { covMat2, ParticleHypothesis::pion()) .value(), }; - std::vector*> vtxList; AdaptiveMultiVertexFitter::State state( *bField, magFieldContext); @@ -457,7 +456,7 @@ BOOST_AUTO_TEST_CASE(adaptive_multi_vertex_fitter_test_athena) { Vertex vtx1(vtxPos1); // Add to vertex list - vtxList.push_back(&vtx1); + state.vertexCollection.push_back(&vtx1); // The constraint vtx for vtx1 Vertex vtx1Constr(vtxPos1); @@ -484,7 +483,7 @@ BOOST_AUTO_TEST_CASE(adaptive_multi_vertex_fitter_test_athena) { Vertex vtx2(vtxPos2); // Add to vertex list - vtxList.push_back(&vtx2); + state.vertexCollection.push_back(&vtx2); // The constraint vtx for vtx2 Vertex vtx2Constr(vtxPos2); @@ -513,7 +512,7 @@ BOOST_AUTO_TEST_CASE(adaptive_multi_vertex_fitter_test_athena) { state.addVertexToMultiMap(vtx2); // Fit vertices - fitter.fit(state, vtxList, linearizer, vertexingOptions); + fitter.fit(state, linearizer, vertexingOptions); auto vtx1Fitted = state.vertexCollection.at(0); auto vtx1PosFitted = vtx1Fitted->position(); From dec82809bac9da58f1b43976048235c36169d02d Mon Sep 17 00:00:00 2001 From: russofel Date: Tue, 14 Nov 2023 11:01:29 +0100 Subject: [PATCH 2/6] improve error handling --- .../Vertexing/AdaptiveMultiVertexFinder.ipp | 12 +++++++-- .../Vertexing/AdaptiveMultiVertexFitter.hpp | 27 ++++++++++++++----- .../include/Acts/Vertexing/VertexingError.hpp | 1 + Core/src/Vertexing/VertexingError.cpp | 2 ++ 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp index 1b6239774b2..42f12c2caf6 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp @@ -24,7 +24,8 @@ auto Acts::AdaptiveMultiVertexFinder::find( // Seed tracks std::vector seedTracks = allTracks; - FitterState_t fitterState(*m_cfg.bField, vertexingOptions.magFieldContext); + FitterState_t fitterState(*m_cfg.bField, vertexingOptions.magFieldContext, + m_logger->cloneWithSuffix("State")); SeedFinderState_t seedFinderState; std::vector>> allVertices; @@ -560,7 +561,14 @@ auto Acts::AdaptiveMultiVertexFinder::deleteLastVertex( // Update fitter state with removed vertex candidate fitterState.removeVertexFromMultiMap(vtx); - fitterState.removeVertexFromCollection(vtx); + // fitterState.vertexCollection contains all vertices that will be fit. When + // we called addVtxToFit, vtx and all vertices that share tracks with vtx were + // added to vertexCollection. Now, we want to refit the same set of vertices + // excluding vx. Therefore, we remove vtx from vertexCollection. + auto removeResult = fitterState.removeVertexFromCollection(vtx); + if (!removeResult.ok()) { + return removeResult.error(); + } for (auto& entry : fitterState.tracksAtVerticesMap) { // Delete all linearized tracks for current (bad) vertex diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp index 76f4cc976bf..ed4fa9d72bd 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp @@ -18,6 +18,7 @@ #include "Acts/Vertexing/LinearizerConcept.hpp" #include "Acts/Vertexing/TrackAtVertex.hpp" #include "Acts/Vertexing/Vertex.hpp" +#include "Acts/Vertexing/VertexingError.hpp" #include "Acts/Vertexing/VertexingOptions.hpp" #include @@ -52,9 +53,12 @@ class AdaptiveMultiVertexFitter { /// @brief The fitter state struct State { State(const MagneticFieldProvider& field, - const Acts::MagneticFieldContext& magContext) + const Acts::MagneticFieldContext& magContext, + std::unique_ptr logger = + getDefaultLogger("AdaptiveMultiVertexFitterState", Logging::INFO)) : ipState(field.makeCache(magContext)), - linearizerState(field.makeCache(magContext)) {} + linearizerState(field.makeCache(magContext)), + m_logger(std::move(logger)) {} // Vertex collection to be fitted std::vector*> vertexCollection; @@ -77,8 +81,10 @@ class AdaptiveMultiVertexFitter { TrackAtVertex> tracksAtVerticesMap; + std::unique_ptr m_logger; + /// @brief Default State constructor - State() = default; + // State() = default; // Adds a vertex to trackToVerticesMultiMap void addVertexToMultiMap(Vertex& vtx) { @@ -99,15 +105,22 @@ class AdaptiveMultiVertexFitter { } } - void removeVertexFromCollection(Vertex& vtxToRemove) { + Result removeVertexFromCollection(Vertex& vtxToRemove) { auto it = std::find(vertexCollection.begin(), vertexCollection.end(), &vtxToRemove); // Check if the value was found before erasing - if (it != vertexCollection.end()) { - // Erase the element if found - vertexCollection.erase(it); + if (it == vertexCollection.end()) { + ACTS_ERROR("vtxToRemove is not part of vertexCollection."); + return VertexingError::InvalidMemoryAccess; } + // Erase the element if found + vertexCollection.erase(it); + return {}; } + + private: + /// Private access to logging instance + const Logger& logger() const { return *m_logger; } }; struct Config { diff --git a/Core/include/Acts/Vertexing/VertexingError.hpp b/Core/include/Acts/Vertexing/VertexingError.hpp index b1ce0da3d8f..e2f6dca9597 100644 --- a/Core/include/Acts/Vertexing/VertexingError.hpp +++ b/Core/include/Acts/Vertexing/VertexingError.hpp @@ -21,6 +21,7 @@ enum class VertexingError { NotConverged, ElementNotFound, NoCovariance, + InvalidMemoryAccess, }; std::error_code make_error_code(Acts::VertexingError e); diff --git a/Core/src/Vertexing/VertexingError.cpp b/Core/src/Vertexing/VertexingError.cpp index 6aeab9d15af..046e6979ac5 100644 --- a/Core/src/Vertexing/VertexingError.cpp +++ b/Core/src/Vertexing/VertexingError.cpp @@ -34,6 +34,8 @@ class VertexingErrorCategory : public std::error_category { return "Unable to find element."; case VertexingError::NoCovariance: return "No covariance provided."; + case VertexingError::InvalidMemoryAccess: + return "Invalid memory access."; default: return "unknown"; } From 9e2b22b2e10ba5ec8d5aa918115f3c5d7c6333ba Mon Sep 17 00:00:00 2001 From: russofel Date: Tue, 14 Nov 2023 11:12:43 +0100 Subject: [PATCH 3/6] remove accidental comment --- Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp index ed4fa9d72bd..dfe8a89c468 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp @@ -84,7 +84,7 @@ class AdaptiveMultiVertexFitter { std::unique_ptr m_logger; /// @brief Default State constructor - // State() = default; + State() = default; // Adds a vertex to trackToVerticesMultiMap void addVertexToMultiMap(Vertex& vtx) { From 948b3e3b965b25e11f58e5d720f2871080563631 Mon Sep 17 00:00:00 2001 From: russofel Date: Tue, 14 Nov 2023 11:47:51 +0100 Subject: [PATCH 4/6] check if no vertices have to be fit --- Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp index 42f12c2caf6..06f87e837d3 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp @@ -577,6 +577,11 @@ auto Acts::AdaptiveMultiVertexFinder::deleteLastVertex( } } + // If no vertices share tracks with vtx we don't need to refit + if (fitterState.vertexCollection.empty()) { + return {}; + } + // Do the fit with removed vertex auto fitResult = m_cfg.vertexFitter.fit(fitterState, m_cfg.linearizer, vertexingOptions); From 1ab563d7eeecf6ab612d229d898740d9a159b44f Mon Sep 17 00:00:00 2001 From: russofel Date: Tue, 14 Nov 2023 12:07:00 +0100 Subject: [PATCH 5/6] throw ElementNotFound --- Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp | 2 +- Core/include/Acts/Vertexing/VertexingError.hpp | 1 - Core/src/Vertexing/VertexingError.cpp | 2 -- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp index dfe8a89c468..0a9c63020d2 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp @@ -111,7 +111,7 @@ class AdaptiveMultiVertexFitter { // Check if the value was found before erasing if (it == vertexCollection.end()) { ACTS_ERROR("vtxToRemove is not part of vertexCollection."); - return VertexingError::InvalidMemoryAccess; + return VertexingError::ElementNotFound; } // Erase the element if found vertexCollection.erase(it); diff --git a/Core/include/Acts/Vertexing/VertexingError.hpp b/Core/include/Acts/Vertexing/VertexingError.hpp index e2f6dca9597..b1ce0da3d8f 100644 --- a/Core/include/Acts/Vertexing/VertexingError.hpp +++ b/Core/include/Acts/Vertexing/VertexingError.hpp @@ -21,7 +21,6 @@ enum class VertexingError { NotConverged, ElementNotFound, NoCovariance, - InvalidMemoryAccess, }; std::error_code make_error_code(Acts::VertexingError e); diff --git a/Core/src/Vertexing/VertexingError.cpp b/Core/src/Vertexing/VertexingError.cpp index 046e6979ac5..6aeab9d15af 100644 --- a/Core/src/Vertexing/VertexingError.cpp +++ b/Core/src/Vertexing/VertexingError.cpp @@ -34,8 +34,6 @@ class VertexingErrorCategory : public std::error_category { return "Unable to find element."; case VertexingError::NoCovariance: return "No covariance provided."; - case VertexingError::InvalidMemoryAccess: - return "Invalid memory access."; default: return "unknown"; } From c255dd9638660e57204523390283ce1678b5788f Mon Sep 17 00:00:00 2001 From: russofel Date: Tue, 14 Nov 2023 12:12:45 +0100 Subject: [PATCH 6/6] suggestions from code review --- .../Acts/Vertexing/AdaptiveMultiVertexFinder.ipp | 5 ++--- .../Acts/Vertexing/AdaptiveMultiVertexFitter.hpp | 16 ++++------------ 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp index 06f87e837d3..64a11706fb3 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp @@ -24,8 +24,7 @@ auto Acts::AdaptiveMultiVertexFinder::find( // Seed tracks std::vector seedTracks = allTracks; - FitterState_t fitterState(*m_cfg.bField, vertexingOptions.magFieldContext, - m_logger->cloneWithSuffix("State")); + FitterState_t fitterState(*m_cfg.bField, vertexingOptions.magFieldContext); SeedFinderState_t seedFinderState; std::vector>> allVertices; @@ -565,7 +564,7 @@ auto Acts::AdaptiveMultiVertexFinder::deleteLastVertex( // we called addVtxToFit, vtx and all vertices that share tracks with vtx were // added to vertexCollection. Now, we want to refit the same set of vertices // excluding vx. Therefore, we remove vtx from vertexCollection. - auto removeResult = fitterState.removeVertexFromCollection(vtx); + auto removeResult = fitterState.removeVertexFromCollection(vtx, logger()); if (!removeResult.ok()) { return removeResult.error(); } diff --git a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp index 0a9c63020d2..030e668097c 100644 --- a/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp +++ b/Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp @@ -53,12 +53,9 @@ class AdaptiveMultiVertexFitter { /// @brief The fitter state struct State { State(const MagneticFieldProvider& field, - const Acts::MagneticFieldContext& magContext, - std::unique_ptr logger = - getDefaultLogger("AdaptiveMultiVertexFitterState", Logging::INFO)) + const Acts::MagneticFieldContext& magContext) : ipState(field.makeCache(magContext)), - linearizerState(field.makeCache(magContext)), - m_logger(std::move(logger)) {} + linearizerState(field.makeCache(magContext)) {} // Vertex collection to be fitted std::vector*> vertexCollection; @@ -81,8 +78,6 @@ class AdaptiveMultiVertexFitter { TrackAtVertex> tracksAtVerticesMap; - std::unique_ptr m_logger; - /// @brief Default State constructor State() = default; @@ -105,7 +100,8 @@ class AdaptiveMultiVertexFitter { } } - Result removeVertexFromCollection(Vertex& vtxToRemove) { + Result removeVertexFromCollection(Vertex& vtxToRemove, + const Logger& logger) { auto it = std::find(vertexCollection.begin(), vertexCollection.end(), &vtxToRemove); // Check if the value was found before erasing @@ -117,10 +113,6 @@ class AdaptiveMultiVertexFitter { vertexCollection.erase(it); return {}; } - - private: - /// Private access to logging instance - const Logger& logger() const { return *m_logger; } }; struct Config {