refactor: Move MLAmbiguitySolver to Core #3272
Conversation
github-actions
bot
added
Component - Core
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3272 +/- ##
=======================================
Coverage 47.46% 47.46%
=======================================
Files 510 510
Lines 30198 30198
Branches 14639 14639
=======================================
Hits 14332 14332
Misses 5335 5335
Partials 10531 10531 ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
Infrastructure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Core/src/TrackFinding/AmbiguityTrackClustering.cpp (1)
Line range hint
26-46: Sound, the clustering logic remains. Approve this change, I do.Elegant, the two-map approach is:
- One map for cluster membership tracks
- Another map for hit-to-cluster mapping maintains
But remember you must: As scale grows larger, memory consumption increases with hit count it does. Consider cleanup of hitToTrack map after clustering, you should.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp(1 hunks)Core/src/TrackFinding/AmbiguityTrackClustering.cpp(1 hunks)Examples/Python/python/acts/examples/reconstruction.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp
🔇 Additional comments (6)
Core/src/TrackFinding/AmbiguityTrackClustering.cpp (1)
24-25: Hmmmm, reversed the iteration order is. Verify the implications we must.
Changed from forward to reverse iteration, the processing order has. Affect cluster assignments this might, as precedence to higher-numbered tracks now given is.
Run this script to understand the impact, we shall:
✅ Verification successful
The documentation reveals that keys in trackMap represent measurement counts, hmmmm. But understand the clustering behavior better, we must. Search for tests and implementation details, we shall.
Safe this change is, the Force tells me
From the documentation revealed, understand we do that trackMap's keys represent measurement counts, ordered they are. Reverse iteration means tracks with more measurements processed first they will be, which logical it is - as tracks with more measurements typically more reliable they are.
The change aligns with the Force, for:
- Documentation states keys are "just there to facilitate the ordering"
- Higher keys mean more measurements, more trustworthy tracks they represent
- Processing higher-quality tracks first, better cluster seeds they make
No disturbance in the Force I sense, as the clustering logic itself unchanged remains. Only the processing order differs, prioritizing tracks with more measurements, wisdom in this approach there is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests and usages of clusterDuplicateTracks
rg -A 5 "clusterDuplicateTracks.*\("
# Look for documentation or comments explaining track ordering
rg -A 5 "trackMap.*order"
Length of output: 2875
Script:
#!/bin/bash
# Search for tests that verify clustering behavior
rg -A 10 "TEST.*cluster.*track" Tests/
# Look for the implementation details of how tracks are processed
ast-grep --pattern 'for (auto track = trackMap.$_(); track != trackMap.$_(); $_track) {
$$$
}'
Length of output: 139
Examples/Python/python/acts/examples/reconstruction.py (5)
1950-1965: Hmmmm, good this configuration looks!
Properly configured, the TrackTruthMatcher is. Follow the established patterns, it does. Clear and consistent, the naming conventions are.
1973-1974: Simple yet effective, these configuration flags are!
Wisely chosen, the boolean flags are. Control the output behavior, they do.
2028-2044: Well balanced, this truth matching configuration is!
Mirror the patterns of its score-based sibling, it does. Properly set, all parameters are.
2049-2054: Clear path for output, this configuration shows!
Follow the established ways of track writing, it does. Wise choice of names, made it has.
Line range hint 1990-2006: Strong with the Force, this ML configuration is!
Properly structured, the configuration is. But verify the integration with Core module, we must.
✅ Verification successful
Strong in the Force, this integration is!
Verified the implementation has been, hmm. The path of the ML Ambiguity Resolution, clear it now is:
- Properly integrated with Core through
Acts::AmbiguityResolutionML, it is - ONNX model handling, well-implemented it appears
- Configuration structure, matches the header definition it does
- Used in multiple chain configurations, proving its readiness it is
In the repository, evidence of proper usage found we have:
- Training scripts for the model, present they are
- Integration tests in full chain scripts, exist they do
- Clear separation between ML and traditional approaches, maintained it is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ML ambiguity resolver integration with Core
# Check for Core integration
ast-grep --pattern 'class AmbiguityResolutionML {
$$$
}'
# Check for ONNX model handling
rg -l "\.onnx" --type python
Length of output: 122
Script:
#!/bin/bash
# Search for AmbiguityResolutionMLAlgorithm class definition
ast-grep --pattern 'class AmbiguityResolutionMLAlgorithm'
# Search for ONNX model usage in Python files
rg -t py "\.onnx"
# Check imports and usage of AmbiguityResolutionMLAlgorithm
rg -t py "AmbiguityResolutionMLAlgorithm"
# Look for Core integration patterns
rg -t py "from acts.core.*ambiguity" -A 2
Length of output: 8515
|
@paulgessinger @CarloVarni It seem the issue with the CI bridge is gone ? Could someone reapprove ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Examples/Python/python/acts/examples/reconstruction.py (1)
Line range hint
2009-2079: Hmm, wise implementation this is, but documentation improve we must!Two-stage approach, clever it is - ML-based followed by greedy resolution. But document the reasoning behind this combination, we should. Help future padawans understand the ways of the Force, it will.
Add docstring explaining:
def addAmbiguityResolutionML(...): """Performs two-stage ambiguity resolution: 1. ML-based initial disambiguation 2. Greedy cleanup for remaining ambiguities This combination leverages ML predictions while ensuring final track collection meets maximum shared hits criteria. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Python/python/acts/examples/reconstruction.py(4 hunks)
🔇 Additional comments (2)
Examples/Python/python/acts/examples/reconstruction.py (2)
1969-1984: Hmmmm, good this implementation looks!
Properly configured the TrackTruthMatcher is, with double matching enabled and appropriate naming conventions followed. A well-structured addition to the score-based ambiguity resolution algorithm this is.
Line range hint 1-2084: Verify the complete removal of DBScan implementation, we must!
Mentioned in PR objectives, removal of DBScan version is. Confirm no lingering references remain, we should.
✅ Verification successful
Lingering DBScan references in the codebase, I sense
In the reconstruction.py file, DBScan configuration still exists:
SeedFilterMLDBScanConfignamedtuple remains defined- DBScan parameters like
epsilonDBScanandminPointsDBScanare still present
However, this relates to seed filtering functionality, not the ambiguity resolution that PR aims to remove. The DBScan version of MLAmbiguitySolver has been successfully removed, as no traces of AmbiguityResolutionMLDBScanConfig in the ambiguity resolution code remain.
Proceed with the changes, we should. Clean is the implementation, mmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DBScan references in the codebase
echo "Searching for DBScan references..."
rg -i "dbscan"
# Specifically check for the old config
echo "Checking for old DBScan configuration..."
ast-grep --pattern 'AmbiguityResolutionMLDBScanConfig'
Length of output: 9599
|
This PR moves the MLAmbiguitySolver to Core, this will allow us to test it more easily with ATLAS in the future. It also removes the DBScan version of this algorithm as it was shown to be way less effective.
Summary by CodeRabbit
New Features
TrackTruthMatcheralgorithm to improve track validation.Bug Fixes
CsvSpacePointWriterclass and its methods.Chores