From 4a4f858d41f3c7f7634155ed881457493149668c Mon Sep 17 00:00:00 2001 From: felix-russo <72298366+felix-russo@users.noreply.github.com> Date: Fri, 29 Sep 2023 15:07:10 +0200 Subject: [PATCH] refactor: rename local variables and improve comments for IVF (#2485) - Rename some variables - Improve some comments - One little change: Previously `hypotVariance` was set to 1 if it was 0, which seems like a random quick fix. I think it is more correct to skip the track in this case because the expression in line 408 would diverge and the if-condition should evaluate to `false`. ![image](https://github.com/acts-project/acts/assets/72298366/2a582bad-88bc-447e-9c8a-35d91670c235) --- .../Acts/Vertexing/IterativeVertexFinder.hpp | 59 +++--- .../Acts/Vertexing/IterativeVertexFinder.ipp | 171 +++++++++--------- .../Vertexing/IterativeVertexFinderTests.cpp | 2 +- 3 files changed, 120 insertions(+), 112 deletions(-) diff --git a/Core/include/Acts/Vertexing/IterativeVertexFinder.hpp b/Core/include/Acts/Vertexing/IterativeVertexFinder.hpp index ace00f7fda9..57b572e0f3e 100644 --- a/Core/include/Acts/Vertexing/IterativeVertexFinder.hpp +++ b/Core/include/Acts/Vertexing/IterativeVertexFinder.hpp @@ -38,16 +38,16 @@ namespace Acts { /// for fitting the single vertex. /// 3.1 If the vertex is a 'good' vertex (i.e. meets requirements) and no /// track reassignment after first fit is required, go to step 4. If vertex -/// is not a good vertex, remove all tracks in perigeesToFit from seedTracks. +/// is not a good vertex, remove all tracks in tracksToFit from seedTracks. /// 3.2 If vertex meets requirements and track reassignment after first fit /// is required, iterate over all previously found vertices ("old vertex") /// and over all their tracksAtVertex. Compare compatibility of each track /// with old vertex and current vertex. If track is more compatible with /// current vertex, remove track from old vertex, put track back to -/// perigeesToFit and refit current vertex with additional track. +/// tracksToFit and refit current vertex with additional track. /// 4. If good vertex, `removeUsedCompatibleTracks` method is called, which /// removes all used tracks that are compatible with the fitted vertex -/// from `perigeesToFit` and `seedTracks`. It also removes outliers tracks +/// from `tracksToFit` and `seedTracks`. It also removes outliers tracks /// from tracksAtVertex if not compatible. /// 5. Add vertex to vertexCollection /// 6. Repeat until no seedTracks are left or max. number of vertices found @@ -94,11 +94,22 @@ class IterativeVertexFinder { /// ImpactPointEstimator IPEstimator ipEst; - // Vertex finder configuration variables + /// Vertex finder configuration variables. + /// Tracks that are within a distance of + /// + /// significanceCutSeeding * sqrt(sigma(d0)^2+sigma(z0)^2) + /// + /// are considered compatible with the vertex. double significanceCutSeeding = 10; - double maximumChi2cutForSeeding = 36.; + double maximumChi2CutForSeeding = 36.; int maxVertices = 50; + + /// Assign a certain fraction of compatible tracks to a different (so-called + /// split) vertex if boolean is set to true. bool createSplitVertices = false; + /// Inverse of the fraction of tracks that will be assigned to the split + /// vertex. E.g., if splitVerticesTrkInvFraction = 2, about 50% of + /// compatible tracks will be assigned to the split vertex. int splitVerticesTrkInvFraction = 2; bool reassignTracksAfterFirstFit = false; bool doMaxTracksCut = false; @@ -188,12 +199,12 @@ class IterativeVertexFinder { const std::vector& seedTracks, const VertexingOptions& vertexingOptions) const; - /// @brief Removes all tracks in perigeesToFit from seedTracks + /// @brief Removes all tracks in tracksToRemove from seedTracks /// - /// @param perigeesToFit Tracks to be removed from seedTracks + /// @param tracksToRemove Tracks to be removed from seedTracks /// @param seedTracks List to remove tracks from - void removeAllTracks(const std::vector& perigeesToFit, - std::vector& seedTracks) const; + void removeTracks(const std::vector& tracksToRemove, + std::vector& seedTracks) const; /// @brief Function for calculating how compatible /// a given track is to a given vertex @@ -210,34 +221,34 @@ class IterativeVertexFinder { State& state) const; /// @brief Function that removes used tracks compatible with - /// current vertex (`myVertex`) from `perigeesToFit` and `seedTracks` - /// as well as outliers from myVertex.tracksAtVertex + /// current vertex (`vertex`) from `tracksToFit` and `seedTracks` + /// as well as outliers from vertex.tracksAtVertex /// - /// @param myVertex Current vertex - /// @param perigeesToFit Tracks used to fit `myVertex` + /// @param vertex Current vertex + /// @param tracksToFit Tracks used to fit `vertex` /// @param seedTracks Tracks used for vertex seeding /// @param vertexingOptions Vertexing options /// @param state The state object Result removeUsedCompatibleTracks( - Vertex& myVertex, - std::vector& perigeesToFit, + Vertex& vertex, + std::vector& tracksToFit, std::vector& seedTracks, const VertexingOptions& vertexingOptions, State& state) const; /// @brief Function that fills vector with tracks compatible with seed vertex /// - /// @param perigeeList List of all available tracks used for seeding + /// @param seedTracks List of all available tracks used for seeding /// @param seedVertex Seed vertex - /// @param perigeesToFitOut Perigees to fit - /// @param perigeesToFitSplitVertexOut Perigees to fit split vertex + /// @param tracksToFitOut Tracks to fit + /// @param tracksToFitSplitVertexOut Tracks to fit to split vertex /// @param vertexingOptions Vertexing options /// @param state The state object - Result fillPerigeesToFit( - const std::vector& perigeeList, + Result fillTracksToFit( + const std::vector& seedTracks, const Vertex& seedVertex, - std::vector& perigeesToFitOut, - std::vector& perigeesToFitSplitVertexOut, + std::vector& tracksToFitOut, + std::vector& tracksToFitSplitVertexOut, const VertexingOptions& vertexingOptions, State& state) const; @@ -246,7 +257,7 @@ class IterativeVertexFinder { /// /// @param vertexCollection Collection of vertices /// @param currentVertex Current vertex to assign tracks to - /// @param perigeesToFit Perigees to fit vector + /// @param tracksToFit Tracks to fit vector /// @param seedTracks Seed tracks vector /// @param origTracks Vector of original track objects /// @param vertexingOptions Vertexing options @@ -256,7 +267,7 @@ class IterativeVertexFinder { Result reassignTracksToNewVertex( std::vector>& vertexCollection, Vertex& currentVertex, - std::vector& perigeesToFit, + std::vector& tracksToFit, std::vector& seedTracks, const std::vector& origTracks, const VertexingOptions& vertexingOptions, diff --git a/Core/include/Acts/Vertexing/IterativeVertexFinder.ipp b/Core/include/Acts/Vertexing/IterativeVertexFinder.ipp index 8c9c6c1fe29..749beefed4b 100644 --- a/Core/include/Acts/Vertexing/IterativeVertexFinder.ipp +++ b/Core/include/Acts/Vertexing/IterativeVertexFinder.ipp @@ -40,45 +40,43 @@ auto Acts::IterativeVertexFinder::find( /// End seeding /// Now take only tracks compatible with current seed // Tracks used for the fit in this iteration - std::vector perigeesToFit; - std::vector perigeesToFitSplitVertex; + std::vector tracksToFit; + std::vector tracksToFitSplitVertex; // Fill vector with tracks to fit, only compatible with seed: - auto res = - fillPerigeesToFit(seedTracks, seedVertex, perigeesToFit, - perigeesToFitSplitVertex, vertexingOptions, state); + auto res = fillTracksToFit(seedTracks, seedVertex, tracksToFit, + tracksToFitSplitVertex, vertexingOptions, state); if (!res.ok()) { return res.error(); } - ACTS_DEBUG("Perigees used for fit: " << perigeesToFit.size()); + ACTS_DEBUG("Number of tracks used for fit: " << tracksToFit.size()); /// Begin vertex fit Vertex currentVertex; Vertex currentSplitVertex; - if (vertexingOptions.useConstraintInFit && !perigeesToFit.empty()) { + if (vertexingOptions.useConstraintInFit && !tracksToFit.empty()) { auto fitResult = m_cfg.vertexFitter.fit( - perigeesToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); + tracksToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); if (fitResult.ok()) { currentVertex = std::move(*fitResult); } else { return fitResult.error(); } - } else if (!vertexingOptions.useConstraintInFit && - perigeesToFit.size() > 1) { + } else if (!vertexingOptions.useConstraintInFit && tracksToFit.size() > 1) { auto fitResult = m_cfg.vertexFitter.fit( - perigeesToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); + tracksToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); if (fitResult.ok()) { currentVertex = std::move(*fitResult); } else { return fitResult.error(); } } - if (m_cfg.createSplitVertices && perigeesToFitSplitVertex.size() > 1) { + if (m_cfg.createSplitVertices && tracksToFitSplitVertex.size() > 1) { auto fitResult = - m_cfg.vertexFitter.fit(perigeesToFitSplitVertex, m_cfg.linearizer, + m_cfg.vertexFitter.fit(tracksToFitSplitVertex, m_cfg.linearizer, vertexingOptions, state.fitterState); if (fitResult.ok()) { currentSplitVertex = std::move(*fitResult); @@ -104,14 +102,14 @@ auto Acts::IterativeVertexFinder::find( nTracksAtVertex >= 2)); if (!isGoodVertex) { - removeAllTracks(perigeesToFit, seedTracks); + removeTracks(tracksToFit, seedTracks); } else { if (m_cfg.reassignTracksAfterFirstFit && (!m_cfg.createSplitVertices)) { // vertex is good vertex here // but add tracks which may have been missed auto result = reassignTracksToNewVertex( - vertexCollection, currentVertex, perigeesToFit, seedTracks, + vertexCollection, currentVertex, tracksToFit, seedTracks, origTracks, vertexingOptions, state); if (!result.ok()) { return result.error(); @@ -121,7 +119,7 @@ auto Acts::IterativeVertexFinder::find( } // end reassignTracksAfterFirstFit case // still good vertex? might have changed in the meanwhile if (isGoodVertex) { - removeUsedCompatibleTracks(currentVertex, perigeesToFit, seedTracks, + removeUsedCompatibleTracks(currentVertex, tracksToFit, seedTracks, vertexingOptions, state); ACTS_DEBUG( @@ -137,9 +135,9 @@ auto Acts::IterativeVertexFinder::find( isGoodSplitVertex = (ndfSplitVertex > 0 && nTracksAtSplitVertex >= 2); if (!isGoodSplitVertex) { - removeAllTracks(perigeesToFitSplitVertex, seedTracks); + removeTracks(tracksToFitSplitVertex, seedTracks); } else { - removeUsedCompatibleTracks(currentSplitVertex, perigeesToFitSplitVertex, + removeUsedCompatibleTracks(currentSplitVertex, tracksToFitSplitVertex, seedTracks, vertexingOptions, state); } } @@ -195,17 +193,16 @@ auto Acts::IterativeVertexFinder::getVertexSeed( } template -void Acts::IterativeVertexFinder::removeAllTracks( - const std::vector& perigeesToFit, +void Acts::IterativeVertexFinder::removeTracks( + const std::vector& tracksToRemove, std::vector& seedTracks) const { - for (const auto& fitPerigee : perigeesToFit) { - const BoundTrackParameters& fitPerigeeParams = - m_extractParameters(*fitPerigee); + for (const auto& trk : tracksToRemove) { + const BoundTrackParameters& params = m_extractParameters(*trk); // Find track in seedTracks auto foundIter = std::find_if(seedTracks.begin(), seedTracks.end(), - [&fitPerigeeParams, this](const auto seedTrk) { - return fitPerigeeParams == m_extractParameters(*seedTrk); + [¶ms, this](const auto seedTrk) { + return params == m_extractParameters(*seedTrk); }); if (foundIter != seedTracks.end()) { // Remove track from seed tracks @@ -257,12 +254,11 @@ Acts::IterativeVertexFinder::getCompatibility( template Acts::Result Acts::IterativeVertexFinder::removeUsedCompatibleTracks( - Vertex& myVertex, - std::vector& perigeesToFit, + Vertex& vertex, std::vector& tracksToFit, std::vector& seedTracks, const VertexingOptions& vertexingOptions, State& state) const { - std::vector> tracksAtVertex = myVertex.tracks(); + std::vector> tracksAtVertex = vertex.tracks(); for (const auto& trackAtVtx : tracksAtVertex) { // Check compatibility @@ -282,16 +278,16 @@ Acts::IterativeVertexFinder::removeUsedCompatibleTracks( ACTS_WARNING("Track trackAtVtx not found in seedTracks!"); } - // Find and remove track from perigeesToFit + // Find and remove track from tracksToFit auto foundFitIter = - std::find_if(perigeesToFit.begin(), perigeesToFit.end(), + std::find_if(tracksToFit.begin(), tracksToFit.end(), [&trackAtVtx](const auto fitTrk) { return trackAtVtx.originalParams == fitTrk; }); - if (foundFitIter != perigeesToFit.end()) { - perigeesToFit.erase(foundFitIter); + if (foundFitIter != tracksToFit.end()) { + tracksToFit.erase(foundFitIter); } else { - ACTS_WARNING("Track trackAtVtx not found in perigeesToFit!"); + ACTS_WARNING("Track trackAtVtx not found in tracksToFit!"); } } // end iteration over tracksAtVertex @@ -299,19 +295,19 @@ Acts::IterativeVertexFinder::removeUsedCompatibleTracks( << seedTracks.size() << " seed tracks left."); // Now start considering outliers - // perigeesToFit that are left here were below + // tracksToFit that are left here were below // m_cfg.cutOffTrackWeight threshold and are hence outliers - ACTS_DEBUG("Number of outliers: " << perigeesToFit.size()); + ACTS_DEBUG("Number of outliers: " << tracksToFit.size()); - const std::shared_ptr myVertexPerigeeSurface = + const std::shared_ptr vertexPerigeeSurface = Surface::makeShared( - VectorHelpers::position(myVertex.fullPosition())); + VectorHelpers::position(vertex.fullPosition())); - for (const auto& myPerigeeToFit : perigeesToFit) { + for (const auto& trk : tracksToFit) { // calculate chi2 w.r.t. last fitted vertex auto result = - getCompatibility(m_extractParameters(*myPerigeeToFit), myVertex, - *myVertexPerigeeSurface, vertexingOptions, state); + getCompatibility(m_extractParameters(*trk), vertex, + *vertexPerigeeSurface, vertexingOptions, state); if (!result.ok()) { return result.error(); @@ -321,11 +317,10 @@ Acts::IterativeVertexFinder::removeUsedCompatibleTracks( // check if sufficiently compatible with last fitted vertex // (quite loose constraint) - if (chi2 < m_cfg.maximumChi2cutForSeeding) { - auto foundIter = std::find_if(seedTracks.begin(), seedTracks.end(), - [&myPerigeeToFit](const auto seedTrk) { - return myPerigeeToFit == seedTrk; - }); + if (chi2 < m_cfg.maximumChi2CutForSeeding) { + auto foundIter = + std::find_if(seedTracks.begin(), seedTracks.end(), + [&trk](const auto seedTrk) { return trk == seedTrk; }); if (foundIter != seedTracks.end()) { // Remove track from seed tracks seedTracks.erase(foundIter); @@ -334,11 +329,9 @@ Acts::IterativeVertexFinder::removeUsedCompatibleTracks( } else { // Track not compatible with vertex // Remove track from current vertex - auto foundIter = - std::find_if(tracksAtVertex.begin(), tracksAtVertex.end(), - [&myPerigeeToFit](auto trk) { - return myPerigeeToFit == trk.originalParams; - }); + auto foundIter = std::find_if( + tracksAtVertex.begin(), tracksAtVertex.end(), + [&trk](auto trkAtVtx) { return trk == trkAtVtx.originalParams; }); if (foundIter != tracksAtVertex.end()) { // Remove track from seed tracks tracksAtVertex.erase(foundIter); @@ -347,46 +340,47 @@ Acts::IterativeVertexFinder::removeUsedCompatibleTracks( } // set updated (possibly with removed outliers) tracksAtVertex to vertex - myVertex.setTracksAtVertex(tracksAtVertex); + vertex.setTracksAtVertex(tracksAtVertex); return {}; } template Acts::Result -Acts::IterativeVertexFinder::fillPerigeesToFit( - const std::vector& perigeeList, +Acts::IterativeVertexFinder::fillTracksToFit( + const std::vector& seedTracks, const Vertex& seedVertex, - std::vector& perigeesToFitOut, - std::vector& perigeesToFitSplitVertexOut, + std::vector& tracksToFitOut, + std::vector& tracksToFitSplitVertexOut, const VertexingOptions& vertexingOptions, State& state) const { - int numberOfTracks = perigeeList.size(); + int numberOfTracks = seedTracks.size(); // Count how many tracks are used for fit int count = 0; - // Fill perigeesToFit vector with tracks compatible with seed - for (const auto& sTrack : perigeeList) { + // Fill tracksToFit vector with tracks compatible with seed + for (const auto& sTrack : seedTracks) { + // If there are only few tracks left, add them to fit regardless of their + // position: if (numberOfTracks <= 2) { - perigeesToFitOut.push_back(sTrack); + tracksToFitOut.push_back(sTrack); ++count; } else if (numberOfTracks <= 4 && !m_cfg.createSplitVertices) { - perigeesToFitOut.push_back(sTrack); + tracksToFitOut.push_back(sTrack); ++count; } else if (numberOfTracks <= 4 * m_cfg.splitVerticesTrkInvFraction && m_cfg.createSplitVertices) { - // Only few tracks left, put them into fit regardless their position - if (count % m_cfg.splitVerticesTrkInvFraction == 0) { - perigeesToFitOut.push_back(sTrack); + if (count % m_cfg.splitVerticesTrkInvFraction != 0) { + tracksToFitOut.push_back(sTrack); ++count; } else { - perigeesToFitSplitVertexOut.push_back(sTrack); + tracksToFitSplitVertexOut.push_back(sTrack); ++count; } } - // still large amount of tracks available, check compatibility + // If a large amount of tracks is available, we check their compatibility + // with the vertex before adding them to the fit: else { - // check first that distance is not too large const BoundTrackParameters& sTrackParams = m_extractParameters(*sTrack); auto distanceRes = m_cfg.ipEst.calculateDistance( vertexingOptions.geoContext, sTrackParams, seedVertex.position(), @@ -399,22 +393,25 @@ Acts::IterativeVertexFinder::fillPerigeesToFit( return VertexingError::NoCovariance; } - double error = + // sqrt(sigma(d0)^2+sigma(z0)^2), where sigma(d0)^2 is the variance of d0 + double hypotVariance = sqrt((*(sTrackParams.covariance()))(eBoundLoc0, eBoundLoc0) + (*(sTrackParams.covariance()))(eBoundLoc1, eBoundLoc1)); - if (error == 0.) { - ACTS_WARNING("Error is zero. Setting to 1."); - error = 1.; + if (hypotVariance == 0.) { + ACTS_WARNING( + "Track impact parameter covariances are zero. Track was not " + "assigned to vertex."); + continue; } - if (*distanceRes / error < m_cfg.significanceCutSeeding) { + if (*distanceRes / hypotVariance < m_cfg.significanceCutSeeding) { if (!m_cfg.createSplitVertices || - count % m_cfg.splitVerticesTrkInvFraction == 0) { - perigeesToFitOut.push_back(sTrack); + count % m_cfg.splitVerticesTrkInvFraction != 0) { + tracksToFitOut.push_back(sTrack); ++count; } else { - perigeesToFitSplitVertexOut.push_back(sTrack); + tracksToFitSplitVertexOut.push_back(sTrack); ++count; } } @@ -428,7 +425,7 @@ Acts::Result Acts::IterativeVertexFinder::reassignTracksToNewVertex( std::vector>& vertexCollection, Vertex& currentVertex, - std::vector& perigeesToFit, + std::vector& tracksToFit, std::vector& seedTracks, const std::vector& /* origTracks */, const VertexingOptions& vertexingOptions, @@ -458,12 +455,12 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( tracksIter++; continue; } - // use original perigee parameter of course - BoundTrackParameters trackPerigee = + // use original perigee parameters + BoundTrackParameters origParams = m_extractParameters(*(tracksIter->originalParams)); // compute compatibility - auto resultNew = getCompatibility(trackPerigee, currentVertex, + auto resultNew = getCompatibility(origParams, currentVertex, *currentVertexPerigeeSurface, vertexingOptions, state); if (!resultNew.ok()) { @@ -472,7 +469,7 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( double chi2NewVtx = *resultNew; auto resultOld = - getCompatibility(trackPerigee, vertexIt, *vertexItPerigeeSurface, + getCompatibility(origParams, vertexIt, *vertexItPerigeeSurface, vertexingOptions, state); if (!resultOld.ok()) { return Result::failure(resultOld.error()); @@ -483,7 +480,7 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( << chi2OldVtx); if (chi2NewVtx < chi2OldVtx) { - perigeesToFit.push_back(tracksIter->originalParams); + tracksToFit.push_back(tracksIter->originalParams); // origTrack was already deleted from seedTracks previously // (when assigned to old vertex) // add it now back to seedTracks to be able to consistently @@ -492,8 +489,8 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( seedTracks.push_back(tracksIter->originalParams); // seedTracks.push_back(*std::find_if( // origTracks.begin(), origTracks.end(), - // [&trackPerigee, this](auto origTrack) { - // return trackPerigee == m_extractParameters(*origTrack); + // [&origParams, this](auto origTrack) { + // return origParams == m_extractParameters(*origTrack); // })); numberOfAddedTracks += 1; @@ -521,17 +518,17 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( // set first to default vertex to be able to check if still good vertex // later currentVertex = Vertex(); - if (vertexingOptions.useConstraintInFit && !perigeesToFit.empty()) { + if (vertexingOptions.useConstraintInFit && !tracksToFit.empty()) { auto fitResult = m_cfg.vertexFitter.fit( - perigeesToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); + tracksToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); if (fitResult.ok()) { currentVertex = std::move(*fitResult); } else { return Result::success(false); } - } else if (!vertexingOptions.useConstraintInFit && perigeesToFit.size() > 1) { + } else if (!vertexingOptions.useConstraintInFit && tracksToFit.size() > 1) { auto fitResult = m_cfg.vertexFitter.fit( - perigeesToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); + tracksToFit, m_cfg.linearizer, vertexingOptions, state.fitterState); if (fitResult.ok()) { currentVertex = std::move(*fitResult); } else { @@ -551,7 +548,7 @@ Acts::IterativeVertexFinder::reassignTracksToNewVertex( nTracksAtVertex >= 2)); if (!isGoodVertex) { - removeAllTracks(perigeesToFit, seedTracks); + removeTracks(tracksToFit, seedTracks); ACTS_DEBUG("Going to new iteration with " << seedTracks.size() << "seed tracks after BAD vertex."); diff --git a/Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp b/Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp index 3955c2c6369..c6b12e58f6e 100644 --- a/Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp +++ b/Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp @@ -599,7 +599,7 @@ BOOST_AUTO_TEST_CASE(iterative_finder_test_athena_reference) { VertexFinder::Config cfg(bFitter, std::move(linearizer), std::move(sFinder), ipEstimator); cfg.maxVertices = 200; - cfg.maximumChi2cutForSeeding = 49; + cfg.maximumChi2CutForSeeding = 49; cfg.significanceCutSeeding = 12; VertexFinder finder(cfg);