From 495c60df45baf716bbdca583e14d586a43657dc5 Mon Sep 17 00:00:00 2001 From: "Alexander J. Pfleger" <70842573+AJPfleger@users.noreply.github.com> Date: Thu, 26 Sep 2024 17:31:35 +0200 Subject: [PATCH] fix: kf+gsf: correct hole-tagging for edge case (#3637) We want to add the `HoleFlag`, when `(precedingMeasurementExists && surfaceIsSensitive)`. We had the case, that we would wrongly always tag holes, when `surfaceIsSensitive && surfaceHasMaterial` because then the check for `precedingMeasurementExists` would have been short-circuited. The physmon does no change, because there we use the ODD which has material and sensitive on different surface, therefore could not be short-circuited. That's why we only see a difference in the generic detector hashes. blocked: - https://github.com/acts-project/acts/pull/3647 --- .../Acts/TrackFitting/KalmanFitter.hpp | 48 ++++++++++--------- .../Acts/TrackFitting/detail/GsfActor.hpp | 11 +++-- .../detail/KalmanUpdateHelpers.hpp | 16 +++++-- Examples/Python/tests/root_file_hashes.txt | 14 +++--- 4 files changed, 51 insertions(+), 38 deletions(-) diff --git a/Core/include/Acts/TrackFitting/KalmanFitter.hpp b/Core/include/Acts/TrackFitting/KalmanFitter.hpp index 1084c55a808..c6cd9b01976 100644 --- a/Core/include/Acts/TrackFitting/KalmanFitter.hpp +++ b/Core/include/Acts/TrackFitting/KalmanFitter.hpp @@ -590,6 +590,11 @@ class KalmanFitter { Result filter(const Surface* surface, propagator_state_t& state, const stepper_t& stepper, const navigator_t& navigator, result_type& result) const { + const bool precedingMeasurementExists = result.measurementStates > 0; + const bool surfaceIsSensitive = + surface->associatedDetectorElement() != nullptr; + const bool surfaceHasMaterial = surface->surfaceMaterial() != nullptr; + // Try to find the surface in the measurement surfaces auto sourceLinkIt = inputMeasurements->find(surface->geometryId()); if (sourceLinkIt != inputMeasurements->end()) { @@ -646,36 +651,33 @@ class KalmanFitter { // the lastTrackIndex. result.lastMeasurementIndex = result.lastTrackIndex; - } else if (surface->associatedDetectorElement() != nullptr || - surface->surfaceMaterial() != nullptr) { + } else if ((precedingMeasurementExists && surfaceIsSensitive) || + surfaceHasMaterial) { // We only create track states here if there is already measurement // detected or if the surface has material (no holes before the first // measurement) - if (result.measurementStates > 0 || - surface->surfaceMaterial() != nullptr) { - auto trackStateProxyRes = detail::kalmanHandleNoMeasurement( - state, stepper, *surface, *result.fittedStates, - result.lastTrackIndex, true, logger(), freeToBoundCorrection); - - if (!trackStateProxyRes.ok()) { - return trackStateProxyRes.error(); - } + auto trackStateProxyRes = detail::kalmanHandleNoMeasurement( + state, stepper, *surface, *result.fittedStates, + result.lastTrackIndex, true, logger(), precedingMeasurementExists, + freeToBoundCorrection); - const auto& trackStateProxy = *trackStateProxyRes; - result.lastTrackIndex = trackStateProxy.index(); + if (!trackStateProxyRes.ok()) { + return trackStateProxyRes.error(); + } - if (trackStateProxy.typeFlags().test(TrackStateFlag::HoleFlag)) { - // Count the missed surface - result.missedActiveSurfaces.push_back(surface); - } + const auto& trackStateProxy = *trackStateProxyRes; + result.lastTrackIndex = trackStateProxy.index(); - ++result.processedStates; - } - if (surface->surfaceMaterial() != nullptr) { - // Update state and stepper with material effects - materialInteractor(surface, state, stepper, navigator, - MaterialUpdateStage::FullUpdate); + if (trackStateProxy.typeFlags().test(TrackStateFlag::HoleFlag)) { + // Count the missed surface + result.missedActiveSurfaces.push_back(surface); } + + ++result.processedStates; + + // Update state and stepper with (possible) material effects + materialInteractor(surface, state, stepper, navigator, + MaterialUpdateStage::FullUpdate); } return Result::success(); } diff --git a/Core/include/Acts/TrackFitting/detail/GsfActor.hpp b/Core/include/Acts/TrackFitting/detail/GsfActor.hpp index c1ed913da29..3bdfc60c785 100644 --- a/Core/include/Acts/TrackFitting/detail/GsfActor.hpp +++ b/Core/include/Acts/TrackFitting/detail/GsfActor.hpp @@ -634,9 +634,11 @@ struct GsfActor { bool doCovTransport) const { const auto& surface = *navigator.currentSurface(state.navigation); + const bool precedingMeasurementExists = result.processedStates > 0; + // Initialize as true, so that any component can flip it. However, all // components should behave the same - bool is_hole = true; + bool isHole = true; auto cmps = stepper.componentIterable(state.stepping); for (auto cmp : cmps) { @@ -647,7 +649,8 @@ struct GsfActor { // now until we measure this is significant auto trackStateProxyRes = detail::kalmanHandleNoMeasurement( singleState, singleStepper, surface, tmpStates.traj, - MultiTrajectoryTraits::kInvalid, doCovTransport, logger()); + MultiTrajectoryTraits::kInvalid, doCovTransport, logger(), + precedingMeasurementExists); if (!trackStateProxyRes.ok()) { return trackStateProxyRes.error(); @@ -656,7 +659,7 @@ struct GsfActor { const auto& trackStateProxy = *trackStateProxyRes; if (!trackStateProxy.typeFlags().test(TrackStateFlag::HoleFlag)) { - is_hole = false; + isHole = false; } tmpStates.tips.push_back(trackStateProxy.index()); @@ -664,7 +667,7 @@ struct GsfActor { } // These things should only be done once for all components - if (is_hole) { + if (isHole) { ++result.measurementHoles; } diff --git a/Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp b/Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp index 06aeae2dd76..332d173f09d 100644 --- a/Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp +++ b/Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp @@ -135,7 +135,7 @@ template auto kalmanHandleNoMeasurement( propagator_state_t &state, const stepper_t &stepper, const Surface &surface, traj_t &fittedStates, const std::size_t lastTrackIndex, bool doCovTransport, - const Logger &logger, + const Logger &logger, const bool precedingMeasurementExists, const FreeToBoundCorrection &freeToBoundCorrection = FreeToBoundCorrection( false)) -> Result { // Add a TrackState entry multi trajectory. This allocates storage for @@ -172,16 +172,24 @@ auto kalmanHandleNoMeasurement( } // Set the track state flags + const bool surfaceHasMaterial = surface.surfaceMaterial() != nullptr; + const bool surfaceIsSensitive = + surface.associatedDetectorElement() != nullptr; auto typeFlags = trackStateProxy.typeFlags(); typeFlags.set(TrackStateFlag::ParameterFlag); - if (surface.surfaceMaterial() != nullptr) { + + if (surfaceHasMaterial) { typeFlags.set(TrackStateFlag::MaterialFlag); } - if (surface.associatedDetectorElement() != nullptr) { + + if (surfaceIsSensitive && precedingMeasurementExists) { ACTS_VERBOSE("Detected hole on " << surface.geometryId()); // If the surface is sensitive, set the hole type flag typeFlags.set(TrackStateFlag::HoleFlag); - } else if (surface.surfaceMaterial() != nullptr) { + } else if (surfaceIsSensitive) { + ACTS_VERBOSE("Skip hole (no preceding measurements) on surface " + << surface.geometryId()); + } else if (surfaceHasMaterial) { ACTS_VERBOSE("Detected in-sensitive surface " << surface.geometryId()); } diff --git a/Examples/Python/tests/root_file_hashes.txt b/Examples/Python/tests/root_file_hashes.txt index cbcf0d85720..54ffad36b9d 100644 --- a/Examples/Python/tests/root_file_hashes.txt +++ b/Examples/Python/tests/root_file_hashes.txt @@ -77,15 +77,15 @@ test_ML_Ambiguity_Solver__performance_ambiML.root: 284ff5c3a08c0b810938e4ac2f8ba test_refitting[odd]__trackstates_gsf_refit.root: 798a5b2e6d4b6d56e73ad107887f310c9463cc739ca1b69dad4a99d3419943ca test_refitting[odd]__tracksummary_gsf_refit.root: 16951808df6363d2acb99e385aec35ad723b634403ca0724a552ae9d3a2ae237 test_refitting[generic]__trackstates_gsf_refit.root: b044574ab5dec9781ca2a1d72095f2393270766365a0e165904ff628191c284a -test_refitting[generic]__tracksummary_gsf_refit.root: 6f8bd054c663197a5f5523e54ebc9695759b671eb14791f964073ed3c8a6f27f -test_truth_tracking_kalman[generic-False-0.0]__trackstates_kf.root: 64a7f26c30f5a70e323dc5d43142a7cc81ad7ef04033095a1ae11d27c41333f0 -test_truth_tracking_kalman[generic-False-0.0]__tracksummary_kf.root: 6f8bd054c663197a5f5523e54ebc9695759b671eb14791f964073ed3c8a6f27f +test_refitting[generic]__tracksummary_gsf_refit.root: 5b9ae5be9e82b2a1fd27d54fae5b9d8e89574f7a04a68cd491d4e3761c8fb834 +test_truth_tracking_kalman[generic-False-0.0]__trackstates_kf.root: 673a2b6a6fe855e8fcc3f0005a8d598ad61fd696177f54b2fa1af36517ac8f44 +test_truth_tracking_kalman[generic-False-0.0]__tracksummary_kf.root: 5b9ae5be9e82b2a1fd27d54fae5b9d8e89574f7a04a68cd491d4e3761c8fb834 test_truth_tracking_kalman[generic-False-1000.0]__trackstates_kf.root: e69d1aacacecc3d7d6619605cdd179d9fb0e19734f7c35345284f9a0e01f36d1 -test_truth_tracking_kalman[generic-False-1000.0]__tracksummary_kf.root: 055a74d2747d360398cc846cc2791204491528896de78cca66b188e3ff530dc1 -test_truth_tracking_kalman[generic-True-0.0]__trackstates_kf.root: 64a7f26c30f5a70e323dc5d43142a7cc81ad7ef04033095a1ae11d27c41333f0 -test_truth_tracking_kalman[generic-True-0.0]__tracksummary_kf.root: 6f8bd054c663197a5f5523e54ebc9695759b671eb14791f964073ed3c8a6f27f +test_truth_tracking_kalman[generic-False-1000.0]__tracksummary_kf.root: 8be4bcc0f05c90ffeb4ea7bbb517adee217550d59c2eed89a48ed3b4c5145c54 +test_truth_tracking_kalman[generic-True-0.0]__trackstates_kf.root: 673a2b6a6fe855e8fcc3f0005a8d598ad61fd696177f54b2fa1af36517ac8f44 +test_truth_tracking_kalman[generic-True-0.0]__tracksummary_kf.root: 5b9ae5be9e82b2a1fd27d54fae5b9d8e89574f7a04a68cd491d4e3761c8fb834 test_truth_tracking_kalman[generic-True-1000.0]__trackstates_kf.root: e69d1aacacecc3d7d6619605cdd179d9fb0e19734f7c35345284f9a0e01f36d1 -test_truth_tracking_kalman[generic-True-1000.0]__tracksummary_kf.root: 055a74d2747d360398cc846cc2791204491528896de78cca66b188e3ff530dc1 +test_truth_tracking_kalman[generic-True-1000.0]__tracksummary_kf.root: 8be4bcc0f05c90ffeb4ea7bbb517adee217550d59c2eed89a48ed3b4c5145c54 test_truth_tracking_kalman[odd-False-0.0]__trackstates_kf.root: 22c9f39a8e9569499205c024f7075d4125aca111c0e580c0320d6d93696062d5 test_truth_tracking_kalman[odd-False-0.0]__tracksummary_kf.root: a6da8b8ca2cd0f09fcbae5d08885fa2aee39990f2f329ef659ef3c260141643a test_truth_tracking_kalman[odd-False-1000.0]__trackstates_kf.root: 214f3ded235a0da8d254e34f72253ee8d6dfa5b7f96040bccd760b51ee85a46f