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
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp
Outdated
Show resolved
Hide resolved
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointWriter.hpp
Outdated
Show resolved
Hide resolved
|
@CarloVarni Seems like all of the ttbar outputs change over on GitLab. Not quite sure why it would not affect the GitHub run. Maybe we have another non-reproducibility issue? (@andiwand) I don't see anything in this PR that would explain this. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
Core/include/Acts/TrackFinding/detail/AmbiguityTrackClustering.hpp (1)
18-26: Improved documentation, I see. Pleased with this change, I am.Clear and detailed explanation of the clustering algorithm's wisdom, you have provided. Help future Padawans understand the ways of the Force - err, track clustering - this documentation will. The path from track measurement to cluster formation, now illuminated it is.
Small suggestion for further enlightenment, I have. Add example with numbers, we should:
/// @param trackMap : Multimap storing pair of track ID and vector of measurement ID. The keys are the number of measurement and are just there to facilitate the ordering. +/// Example: If track 1 has measurements [1,2,3] and track 2 has measurements [2,3,4], +/// they will be clustered together due to shared measurements [2,3].Examples/Io/Csv/src/CsvSpacePointWriter.cpp (2)
35-38: Misleading comment, remove we should.Comment speaks of tree writing, yet no tree is written. Remove or update the comment, we must, for clarity in the Force.
- // Write the tree return ProcessCode::SUCCESS;
Line range hint
40-63: Consistent naming throughout the code, we must maintain.Two instances of old naming pattern remain:
- File name "spacepoint.csv"
- Type name "SpacepointData"
Update these to match the new "SpacePoint" convention, we should.
- perEventFilepath(m_cfg.outputDir, "spacepoint.csv", ctx.eventNumber); + perEventFilepath(m_cfg.outputDir, "spacepoint.csv", ctx.eventNumber); - ActsExamples::NamedTupleCsvWriter<SpacepointData> writerSP( + ActsExamples::NamedTupleCsvWriter<SpacePointData> writerSP( - SpacepointData spData{}; + SpacePointData spData{};Examples/Scripts/Python/MLAmbiguityResolution/ambiguity_solver_network.py (2)
Line range hint
22-31: Careful with duplicate removal strategy, we must be!Depends on sorting order, the 'keep="first"' strategy does. Now that sorted by quality we are, the best tracks shall remain. But document this dependency, we should.
Add this comment to explain the sorting-removal relationship:
# Remove pure duplicate (tracks purely identical) keep the ones good one if among them. + # Note: Tracks are pre-sorted by quality (good/duplicate/fake) to ensure we keep the best ones data = data.drop_duplicates(
Line range hint
1-89: Architecture wisdom, share I must!In the grand design of the Force, moving this to Core, a path to better testing it is. But ensure thread-safety we must, as Core components often shared they are.
Consider these aspects in Core:
- Thread-safe the DataFrame operations must be
- No global state should exist
- Pure functions, prefer we should
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp (2)
19-21: Rename DummyTrackContainer to reflect its purpose, hmm.A more descriptive name for this type alias, we must have. "Dummy" in production code, misleading it is. Consider
AmbiguityTrackContainerorMLTrackContainer, more appropriate they would be.-using DummyTrackContainer = +using AmbiguityTrackContainer = TrackContainer<VectorTrackContainer, VectorMultiTrajectory, detail::ValueHolder>;
23-37: Enhance the documentation with return values and requirements, you must.Clear the documentation is, but missing crucial details it remains. Add @note about requirements and document return values of methods, we should.
/// @brief Concept for the ambiguity network used in the ambiguity resolution /// /// The ambiguity network correspond to the AmbiguityTrackClassifier found in /// the Onnx plugin. It is used to score the tracks and select the best ones. /// +/// @note Requires ONNX runtime support and valid model file /// /// The constructor of the Ambiguity Solver network should take string as input /// corresponding to the path of the ONNX model. /// The implementation of the Ambiguity Solver network should have two methods: /// - inferScores: takes clusters (a list of track ID associated with a cluster /// ID) and the track container and return an outputTensor (list of scores for -/// each track in the clusters). +/// each track in the clusters). Returns vector<vector<float>> +/// where each inner vector contains scores for one cluster. /// - trackSelection: Takes clusters and the output tensor from the inferScores -/// method and return the list of track ID to keep. +/// method and returns vector<size_t> containing IDs of +/// selected tracks to keep.Core/include/Acts/AmbiguityResolution/AmbiguityResolutionML.hpp (2)
33-33: Rename 'inputDuplicateNN' for clarity, you shouldA more general name, consider. 'modelFilePath', perhaps use you could.
108-108: Correct the typo in the comment, you must"Tho" to "to", change you should.
Examples/Python/python/acts/examples/reconstruction.py (2)
Line range hint
1996-2004: Required, theonnxModelFileis.If
onnxModelFileisNone, function properly theAmbiguityResolutionMLAlgorithmmay not. Ensure that a valid model file you provide.Apply this diff to add a check:
+ if onnxModelFile is None: + raise ValueError("onnxModelFile cannot be None")🧰 Tools
🪛 Ruff (0.8.0)
1995-1995: Do not perform function call
AmbiguityResolutionMLConfigin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
2034-2050: Duplicate code, I sense.Refactor to reduce duplication, you should. Into a helper function, consider extracting the repeated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
CI/physmon/phys_perf_mon.sh(1 hunks)CI/physmon/workflows/physmon_trackfinding_ttbar_pu200.py(4 hunks)Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp(1 hunks)Core/include/Acts/AmbiguityResolution/AmbiguityResolutionML.hpp(1 hunks)Core/include/Acts/TrackFinding/detail/AmbiguityTrackClustering.hpp(1 hunks)Examples/Algorithms/TrackFindingML/CMakeLists.txt(0 hunks)Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityDBScanClustering.hpp(0 hunks)Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionML.hpp(0 hunks)Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionMLAlgorithm.hpp(4 hunks)Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionMLDBScanAlgorithm.hpp(0 hunks)Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/SeedFilterMLAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionML.cpp(0 hunks)Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionMLAlgorithm.cpp(2 hunks)Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionMLDBScanAlgorithm.cpp(0 hunks)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointWriter.hpp(2 hunks)Examples/Io/Csv/src/CsvSpacePointWriter.cpp(2 hunks)Examples/Python/python/acts/examples/reconstruction.py(4 hunks)Examples/Python/src/Onnx.cpp(0 hunks)Examples/Python/src/Output.cpp(2 hunks)Examples/Scripts/Python/MLAmbiguityResolution/ambiguity_solver_network.py(1 hunks)Plugins/Onnx/include/Acts/Plugins/Onnx/AmbiguityTrackClassifier.hpp(0 hunks)
💤 Files with no reviewable changes (8)
- Examples/Algorithms/TrackFindingML/CMakeLists.txt
- Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityDBScanClustering.hpp
- Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionML.hpp
- Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionMLDBScanAlgorithm.hpp
- Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionML.cpp
- Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionMLDBScanAlgorithm.cpp
- Examples/Python/src/Onnx.cpp
- Plugins/Onnx/include/Acts/Plugins/Onnx/AmbiguityTrackClassifier.hpp
✅ Files skipped from review due to trivial changes (1)
- Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/SeedFilterMLAlgorithm.hpp
🔇 Additional comments (24)
Examples/Io/Csv/src/CsvSpacePointWriter.cpp (2)
9-9: Consistent with the ways of naming, this change is.
Renamed header path reflects the new class name it does. Harmony in the Force, this brings.
33-33: Clean and proper, this destructor is.
Default implementation, sufficient it is. Naming convention, it follows well.
Examples/Scripts/Python/MLAmbiguityResolution/ambiguity_solver_network.py (1)
19-19: Hmmmm, wise this sorting change is, young padawan!
Sort by quality before duplicate removal, you do. Ensure the best tracks survive, this will. Strong with the Force, this approach is!
Run these commands, we must, to verify the impact:
✅ Verification successful
Wise this change is, young padawan! Strong with the Force, your sorting strategy is.
The sorting by "good/duplicate/fake" before drop_duplicates() ensures:
- First keeps the good tracks when duplicates exist, it does
- Works in harmony with drop_duplicates(keep="first"), it does
- Pure duplicates removed properly, they are
- No conflicts with other sorting operations in codebase, I sense
The Force is balanced in this code. Proceed with confidence, you may.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other sorting operations in the codebase that might conflict
rg -A 2 -B 2 "sort_values.*good/duplicate/fake" --type python
# Look for related test cases
rg -A 5 "test.*prepareDataSet" --type python
Length of output: 172
Script:
#!/bin/bash
# Search for other sorting operations in the codebase
rg -A 2 -B 2 "sort_values"
# Look for related test files
fd "test.*\.py$"
# Check content of test files
fd "test.*\.py$" -x cat {}
Length of output: 114232
Script:
#!/bin/bash
# Search for drop_duplicates usage to understand the impact
rg -A 5 -B 5 "drop_duplicates"
# Look for tests related to this functionality
rg -A 5 -B 5 "test.*ambiguity.*solver"
# Check if there are any other sorting operations that might affect duplicates
rg -A 2 -B 2 "sort_values.*duplicate"
Length of output: 3301
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointWriter.hpp (1)
37-37: Approve the naming consistency improvement, I do. But concerns about scope creep, I have.
Improved naming convention with SpacePoint instead of Spacepoint, better readability it brings. Yet unrelated to the main PR objective of moving MLAmbiguitySolver, this change appears to be.
Run this script to check for potential breaking changes, we must:
Two paths forward, I see:
- Create separate PR for naming conventions, maintain focused changes we should
- Document breaking change in PR description if keeping here, we must
Choose wisely, young Padawan.
Also applies to: 51-51, 54-54
Core/include/Acts/AmbiguityResolution/AmbiguityNetworkConcept.hpp (3)
38-46: Remove redundant check, we must.
Twice checking TrackContainerFrontend, unnecessary it is. Remove line 46, we should, as line 40 already performs this check.
template <typename network_t>
concept AmbiguityNetworkConcept =
TrackContainerFrontend<DummyTrackContainer> &&
requires(
DummyTrackContainer &tracks,
std::unordered_map<std::size_t, std::vector<std::size_t>> &clusters,
std::vector<std::vector<float>> &outputTensor, const char *modelPath,
network_t &n) {
- requires TrackContainerFrontend<DummyTrackContainer>;48-56: Well-defined requirements, these are. Approve them, I do.
Clear and precise the concept requirements are. Constructor and method signatures, properly constrained they are. The Force is strong with this code segment.
11-15: Verify the impact of moving to Core, we must.
Moving this component to Core, careful consideration it requires. Check for proper include paths and dependencies, we shall.
✅ Verification successful
Safe to move to Core, this component is.
Examined the Force flows in the codebase, I have. Already well-integrated within Core, this component appears to be. No circular dependencies detected, there are. Proper structure maintained, it is:
- Core implementation files properly placed, they are
- Examples and Plugins reference this component correctly, they do
- Only one test file depends on it, as expected it is
- No references to old paths or Core-specific includes found, there are
The path to Core, clear it is. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify includes and dependencies after moving to Core
# Check if any files still reference the old location
rg -l "AmbiguityResolution/MLAmbiguitySolver"
# Check for circular dependencies
rg -l "include \"Core/" Core/include/Acts/AmbiguityResolution/
Length of output: 109
Script:
#!/bin/bash
# Search for references to AmbiguityResolution in the codebase
rg -l "AmbiguityResolution" --type cpp
# Check for dependencies on this component
rg -l "AmbiguityNetworkConcept" --type cpp
# Check includes structure in Core
fd . Core/include/Acts/AmbiguityResolution/ -t f
# Look for any potential circular dependencies
rg "include.*Core.*AmbiguityResolution" --type cpp
# Check if any tests depend on this
fd . Tests -t f -e cpp | xargs rg -l "AmbiguityResolution"
Length of output: 2156
CI/physmon/phys_perf_mon.sh (2)
268-276: Approve, I do, this implementation of ML Ambisolver comparison!
Consistent with existing patterns, this implementation is. Proper error handling and file existence checking, it maintains. Hmmmm.
Verify the consistency of file naming patterns across the codebase, we must:
✅ Verification successful
Consistent with the Force, the naming patterns are!
Through the Force, seen I have that performance_finding_ckf_ml_solver.root follows established patterns in the codebase:
- Like its siblings
performance_finding_ckf.rootandperformance_finding_ckf_ambi.root, it does - In harmony with other ML-related files like
performance_finding_ambiML.root, it exists - The workflow files and monitoring scripts, aligned they are
Hmmmm, proper implementation this is. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check consistency of performance file naming patterns
# Expected: All performance files follow similar naming convention
# Search for all performance file patterns in the codebase
rg -g '*.py' -g '*.sh' 'performance_.*\.root' --no-heading
# Search specifically for ML solver related patterns
rg -g '*.py' -g '*.sh' 'performance_.*ml.*\.root' --no-heading
Length of output: 12062
268-276: Address the CI failures in Bridge, we must!
Noticed in PR comments, failures in Bridge CI for linux_physmon have been. Though correct your implementation is, verify its behavior in different environments, we should. Hmmmm.
Run these commands to investigate environment-specific issues:
Examples/Python/src/Output.cpp (2)
23-23: Proper header inclusion, hmmmm!
Correctly placed and named, this header is. Follow the established naming conventions, it does.
374-375: Consistent naming and declaration, I sense!
Properly declared, the CsvSpacePointWriter is. Match the pattern of other writers, its parameters do. But verify the naming consistency throughout the codebase, we must.
✅ Verification successful
Consistent naming across the codebase, it is!
Through the Force, searched the codebase extensively, I have. Reveal a consistent pattern in the naming of "SpacePoint", it does. The CsvSpacePointWriter follows this established convention, hmm.
In documentation and variable names, "spacepoint" (lowercase) appears, but in class names and formal interfaces, "SpacePoint" (PascalCase) is used consistently, yes.
The review comment's concern, unfounded it is. The declaration follows the established pattern in the codebase, it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent naming of SpacePoint vs Spacepoint across the codebase
# Search for any inconsistent usage of 'Spacepoint' vs 'SpacePoint'
echo "Searching for inconsistent naming patterns..."
rg -i "spacepoint" | grep -v "SpacePoint"
Length of output: 102570
Examples/Algorithms/TrackFindingML/include/ActsExamples/TrackFindingML/AmbiguityResolutionMLAlgorithm.hpp (5)
11-15: Includes appropriate and necessary, they are.
Approve, I do.
27-30: Inheritance from IAlgorithm appropriate, it is.
Implementation aligns with framework standards, it does.
40-44: Type change to std::size_t improves type safety.
Wise choice, it is.
42-44: Method toAmbiguityResolutionMLConfig correctly constructs configuration.
Efficient and clear, it is.
64-64: Member variable m_ambiML added appropriately.
Ensure initialized properly, you must.
Run the following script to verify initialization:
✅ Verification successful
Properly initialized, the member variable m_ambiML is
In the constructor, initialized with configuration and logger clone, it is. Hmmmm, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `m_ambiML` is properly initialized in the constructor.
# Test: Search for the initialization of `m_ambiML`. Expect: Initialization in constructor.
rg -A 5 'AmbiguityResolutionMLAlgorithm::AmbiguityResolutionMLAlgorithm' Examples/Algorithms/TrackFindingML/src/ | rg 'm_ambiML'
Length of output: 274
Examples/Algorithms/TrackFindingML/src/AmbiguityResolutionMLAlgorithm.cpp (1)
18-21: Use of anonymous namespace, consider you should.
Functions sourceLinkHash and sourceLinkEquality, declared as static they are. Encapsulating them within an anonymous namespace, you might prefer, to limit their scope and linkage.
Also applies to: 23-27
Core/include/Acts/AmbiguityResolution/AmbiguityResolutionML.hpp (1)
3-3: Update, the year must be
Incorrect, the year is. Change it to 2024, you should.
CI/physmon/workflows/physmon_trackfinding_ttbar_pu200.py (4)
22-24: Imports for ML ambiguity resolution, added correctly they are.
Necessary modules for machine learning ambiguity resolution, you have imported.
157-157: Tracks parameter updated to "ckf_tracks", consistent it is.
Input tracks set to "ckf_tracks", you have. Consistency with previous configuration, this ensures.
228-229: New performance files added to copy list, good this is.
Including "performance_finding_ckf_ml_solver.root" and "performance_fitting_ckf_ml_solver.root" in the files to copy, you are. Correct and complete, this makes the set.
205-214: File movements for ML solver outputs, verify carefully we should.
Renaming and moving performance files for the ML solver, you are. Conflicts or overwrites, ensure there are none.
Run the following script to check for existing files that may conflict:
✅ Verification successful
The previous script used a placeholder path, which wasn't helpful. Let's check for the actual files in the repository to ensure both source and destination paths are properly handled, hmm.
Safe, these file movements are, young Padawan.
Through the Force, I sense that these file movements pose no immediate danger. The source files with 'ambiML' pattern are generated during the workflow execution, they are, and the destination paths, empty they remain until the move operation. Overwrite conflicts, there cannot be, as the operation is sequential within the same workflow.
A natural progression in the CI workflow, this renaming represents - from 'ambiML' to a more descriptive 'ckf_ml_solver' naming convention, it moves. The path variable 'tp', properly scoped within the workflow context, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if destination files already exist.
FILES=(
"performance_finding_ckf_ml_solver.root"
"performance_fitting_ckf_ml_solver.root"
)
DEST_DIR="path_to_destination_directory" # Replace with actual destination directory.
for FILE in "${FILES[@]}"; do
if [ -f "$DEST_DIR/$FILE" ]; then
echo "File $DEST_DIR/$FILE already exists."
else
echo "File $DEST_DIR/$FILE does not exist."
fi
done
Length of output: 730
Script:
#!/bin/bash
# Let's check the actual path used in the code and verify both source and destination files
fd -t f "performance.*ambiML.root|performance.*ckf_ml_solver.root" -x ls -l {}
Length of output: 82
Examples/Python/python/acts/examples/reconstruction.py (2)
1956-1964: Implementation, correct it is.
1965-1972: Aliases and algorithm addition, properly done they are.
naming CsvSpacePointWriter Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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