Feature: source link filtering#4186
Feature: source link filtering#4186hupereir wants to merge 1 commit intosPHENIX-Collaboration:masterfrom
Conversation
…move measurements whether or not we run the direct navigations. GetSurfaceVect, as used for the direct navigation, does not apply any filtering.
📝 WalkthroughWalkthroughThe changes refactor source link filtering and surface extraction logic in PHActsTrkFitter. A new Changes
✨ Finishing Touches
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // filter sourcelinks to remove detectors that we don't want to include in the fit | ||
| sourceLinks = filterSourceLinks( sourceLinks ); | ||
|
|
There was a problem hiding this comment.
Unconditional filterSourceLinks call changes behaviour for non-default flag combinations.
Previously, all filtering lived inside the old getSurfaceVector, which was only reached when m_fitSiliconMMs || m_directNavigation. After this change, the micromegas exclusion (!m_useMicromegas) and the silicon-only cut (m_forceSiOnlyFit) are applied to every fit path, including the standard TPC fit.
For the default settings (m_useMicromegas = true, m_forceSiOnlyFit = false) there is no change. However, any workflow that sets m_useMicromegas = false without enabling directed navigation will now silently drop micromegas clusters from the fit, which was not the case before.
Given the PR's stated intent ("filtering can be applied regardless of directed fit"), this appears deliberate, but it warrants a brief comment near line 547 to make the scope of the change explicit for future maintainers.
💬 Suggested in-code clarification
- // filter sourcelinks to remove detectors that we don't want to include in the fit
+ // filter sourcelinks to remove detectors excluded from the fit;
+ // applied unconditionally (i.e. independent of directed-navigation mode)
sourceLinks = filterSourceLinks( sourceLinks );| SourceLinkVec filtered; | ||
| for (const auto& sl : sourceLinks) | ||
| { | ||
| const ActsSourceLink asl = sl.get<ActsSourceLink>(); | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << "SL available on : " << asl.geometryId() << std::endl; | ||
| } | ||
|
|
||
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | ||
| if (m_fitSiliconMMs) | ||
| { | ||
| // skip TPC surfaces | ||
| if (m_tGeometry->maps().isTpcSurface(surf)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // also skip micromegas surfaces if not used | ||
| if (m_tGeometry->maps().isMicromegasSurface(surf) && !m_useMicromegas) | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
| // skip TPC surfaces for fitSilicon MMs | ||
| if (m_tGeometry->maps().isTpcSurface(surf) && m_fitSiliconMMs) | ||
| { continue; } | ||
|
|
||
| if (m_forceSiOnlyFit) | ||
| { | ||
| if (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
| // skip micromegas surfaces if not used | ||
| if (m_tGeometry->maps().isMicromegasSurface(surf) && !m_useMicromegas) | ||
| { continue; } | ||
|
|
||
| // skip everything but silicons if only silicon fit is required | ||
| if (m_forceSiOnlyFit && (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) ) | ||
| { continue; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine isTpcSurface and isMicromegasSurface implementations
rg -n "void.*isTpcSurface|bool.*isTpcSurface" --type=cpp -A10 -B2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2240
🏁 Script executed:
# Also search for isMicromegasSurface
rg -n "void.*isMicromegasSurface|bool.*isMicromegasSurface" --type=cpp -A10 -B2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2202
🏁 Script executed:
# Check the class containing these methods - likely in maps
fd -e h -e hpp | xargs rg -l "isTpcSurface|isMicromegasSurface" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 122
🏁 Script executed:
# Search for findSurface return type and behavior
rg -n "findSurface" --type=cpp --type=hpp -B2 -A5 | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Search for findSurface definition in Acts geometry code
rg -n "findSurface" --type=cpp --type=h -B5 -A10 | grep -A10 "findSurface.*{" | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if findSurface is documented or used with null checks elsewhere
rg -n "findSurface" offline/packages/trackreco/PHActsTrkFitter.cc -B3 -A3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 674
🏁 Script executed:
# Search Acts library to understand findSurface behavior
fd -type f -name "*.h" | xargs rg -l "findSurface" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 355
🏁 Script executed:
# Look for TrackingGeometry or tGeometry type to understand findSurface
rg -n "tGeometry" offline/packages/trackreco/PHActsTrkFitter.cc | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 501
🏁 Script executed:
# Check if there are any examples in the codebase where findSurface result is checked for null
rg -n "findSurface" -B2 -A2 | grep -A2 -B2 "if.*nullptr\|if.*!.*surf\|if.*surf.*==" | head -30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
Add null-check after findSurface to prevent undefined behavior.
isTpcSurface and isMicromegasSurface both dereference their argument without null-checking (lines 34 and 39 of ActsSurfaceMaps.cc). Since findSurface returns nullptr when a geometry ID is absent from the Acts geometry (e.g., stale clusters or geometry mismatches), passing the unchecked result to these functions at lines 976, 980, and 984 causes undefined behavior.
Suggested fix
const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId());
+ if (!surf) { continue; }
// skip TPC surfaces for fitSilicon MMs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SourceLinkVec filtered; | |
| for (const auto& sl : sourceLinks) | |
| { | |
| const ActsSourceLink asl = sl.get<ActsSourceLink>(); | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << "SL available on : " << asl.geometryId() << std::endl; | |
| } | |
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | |
| if (m_fitSiliconMMs) | |
| { | |
| // skip TPC surfaces | |
| if (m_tGeometry->maps().isTpcSurface(surf)) | |
| { | |
| continue; | |
| } | |
| // also skip micromegas surfaces if not used | |
| if (m_tGeometry->maps().isMicromegasSurface(surf) && !m_useMicromegas) | |
| { | |
| continue; | |
| } | |
| } | |
| // skip TPC surfaces for fitSilicon MMs | |
| if (m_tGeometry->maps().isTpcSurface(surf) && m_fitSiliconMMs) | |
| { continue; } | |
| if (m_forceSiOnlyFit) | |
| { | |
| if (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) | |
| { | |
| continue; | |
| } | |
| } | |
| // skip micromegas surfaces if not used | |
| if (m_tGeometry->maps().isMicromegasSurface(surf) && !m_useMicromegas) | |
| { continue; } | |
| // skip everything but silicons if only silicon fit is required | |
| if (m_forceSiOnlyFit && (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) ) | |
| { continue; } | |
| SourceLinkVec filtered; | |
| for (const auto& sl : sourceLinks) | |
| { | |
| const ActsSourceLink asl = sl.get<ActsSourceLink>(); | |
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | |
| if (!surf) { continue; } | |
| // skip TPC surfaces for fitSilicon MMs | |
| if (m_tGeometry->maps().isTpcSurface(surf) && m_fitSiliconMMs) | |
| { continue; } | |
| // skip micromegas surfaces if not used | |
| if (m_tGeometry->maps().isMicromegasSurface(surf) && !m_useMicromegas) | |
| { continue; } | |
| // skip everything but silicons if only silicon fit is required | |
| if (m_forceSiOnlyFit && (m_tGeometry->maps().isMicromegasSurface(surf) || m_tGeometry->maps().isTpcSurface(surf)) ) | |
| { continue; } |
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | ||
| surfaces.push_back(surf); | ||
| } |
There was a problem hiding this comment.
Null Surface* can be silently inserted into the directed-navigation sequence.
findSurface returns nullptr for unrecognised geometry IDs, and the result is pushed into surfaces without any guard. The directed Kalman fitter dereferences every element of surfSequence, so a single null entry will crash.
🛡️ Suggested guard
const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId());
+ if (!surf) { continue; } // drop source links whose surface is absent in the geometry
surfaces.push_back(surf);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | |
| surfaces.push_back(surf); | |
| } | |
| const auto* const surf = m_tGeometry->geometry().tGeometry->findSurface(asl.geometryId()); | |
| if (!surf) { continue; } // drop source links whose surface is absent in the geometry | |
| surfaces.push_back(surf); | |
| } |
1 participant
This PR splits the functionalities of GetSurfaceVect() method in two.
The first FilterSourceLinks new method filters the source links to remove measurements from detectors that we don't want to include in the fit (like TPC in TPOT-Silicon fit; TPC and TPOT in silicon only fit, etc.).
this can now be applied whether or not one uses the directed fit.
The second method, GetSurfaceVect, specific to the directed fit, selects the list of surfaces from the filtered sourcelinks
this should have no impact on the current workflow, but should make it easy to (for instance) remove Micromegas clusters from the default ACTS fit, even if the clusters are on the track.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
PR Summary: Source Link Filtering for ACTS Track Fitter
Motivation
This PR refactors the ACTS track fitting algorithm to separate the concern of filtering detector measurements from the surface extraction logic. Previously, filtering of source links (measurements to be excluded from track fits) was tightly coupled with other parts of the track fitting workflow. The change enables flexible exclusion of specific detector measurements independent of whether directed navigation is used, making it easier to exclude detectors like Micromegas clusters from default ACTS fits.
Key Changes
New method
filterSourceLinks(): Implements centralized filtering logic that removes source links based on current fit configuration:New method
getSurfaceVector(): Extracts surfaces directly from source links without applying filters, used specifically for directed navigation pathControl flow restructuring: Filtering is now applied early in the track fitting loop before surface checking, removing empty source link lists before proceeding with surface extraction
API changes:
SourceLinkVec filterSourceLinks(const SourceLinkVec& sourceLinks) constSurfacePtrVec getSurfaceVector(const SourceLinkVec& sourceLinks) constPotential Risk Areas
m_fitSiliconMMs,m_forceSiOnlyFit,m_useMicromegas)—ensure combinations are well-testedIntended Impact
The PR is designed to be non-breaking for current workflows while enabling future flexibility in detector measurement exclusion strategies.
Note: AI-generated analysis aids the review but careful manual inspection of the filtering logic and control flow changes is recommended.