refactor: Create particle/simhit to measurement maps in DigitizationAlgorithm in Examples
#3944
Conversation
…Algorithm` in Examples
WalkthroughEnhancements to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
![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 (16)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.hpp (1)
52-53: Wise choice, the new data structure is! Approve this change, I do.From
HitParticlesMaptoInverseMultimap<SimBarcode>, evolved the type has. Better supports the bidirectional relationship tracking, it does. Aligned with the force of good practices, this change is.Remember you must, that with great power comes great responsibility. When using
InverseMultimap, ensure proper cleanup of mappings you do, or memory leaks to the dark side lead they might.Examples/Io/Root/include/ActsExamples/Io/Root/SeedingPerformanceWriter.hpp (1)
83-84: Fix the plural form in string literal, you must!Hmmmm... A disturbance in the force, I sense. The string literal "InputMeasurementParticlesMaps" uses plural form, while the type
MeasurementParticlesMapspeaks of singular. Consistency in naming, crucial it is!Apply this change, you should:
ReadDataHandle<MeasurementParticlesMap> m_inputMeasurementParticlesMap{ - this, "InputMeasurementParticlesMaps"}; + this, "InputMeasurementParticlesMap"};Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.cpp (1)
121-121: Improve the comments and track handling, we must!More descriptive comments about measurement handling, beneficial they would be. Consider:
- track.reserve(measurements.size()); + // Reserve space for all measurements associated with this particle + track.reserve(measurements.size()); - // Loop over the measurement index on the proto track to find the space - // points + // Iterate through measurement indices to locate corresponding space points + // for constructing the track trajectoryAlso applies to: 134-136
Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderNTupleWriter.hpp (1)
33-34: Architectural wisdom, share I must.Through the Force, see I do that this change part of larger refactoring journey is. Consistent terminology across the codebase, strengthen the bonds between components it will. Like the Jedi Council working in harmony, your code structure becomes.
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvTrackWriter.hpp (1)
43-60: Clear documentation, you have provided. Strong with the Force, it is.Well-structured and documented configuration parameters, these are. The explicit unit specification for ptMin using
Acts::UnitConstants::GeV, clarity it brings.Consider grouping related parameters together, young Padawan:
struct Config { /// Input track collection std::string inputTracks; /// Input hit-particles map collection std::string inputMeasurementParticlesMap; /// where to place output files std::string outputDir; /// name of the output files std::string fileName = "CKFtracks.csv"; /// floating point precision std::size_t outputPrecision = 6; /// Track quality parameters /// Min number of measurements std::size_t nMeasurementsMin = 7; /// Min pt of tracks double ptMin = 1 * Acts::UnitConstants::GeV; /// Truth matching parameters /// Only write truth matched tracks bool onlyTruthMatched = false; /// Probability threshold for fake tracks double truthMatchProbMin = 0.5; };Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackParameterWriter.hpp (2)
34-35: Simplified inheritance structure, observe I do.Cleaner and more direct, the inheritance has become. From complexity to simplicity, we have moved. Good practice, this is.
Consider documenting the rationale for direct inheritance in the class description, you should. Help future padawans understand the design choice, it will.
Line range hint
34-150: Structural improvements suggest, I must.Many track parameter members, this class has. Group them into logical structs, we could. Reduce duplication and improve organization, it would.
Consider this structure, you should:
struct TrackParameters { float loc0{NaNfloat}; float loc1{NaNfloat}; float phi{NaNfloat}; // ... other parameters }; class RootTrackParameterWriter { TrackParameters m_params; TrackParameters m_truthParams; // ... rest of the class };Cleaner and more maintainable, your code will become. Easier to manage related parameters together, it will be.
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp (1)
107-108: Wise is the change to MeasurementSimHitsMap, but document it we should.Changed from HitSimHitsMap to MeasurementSimHitsMap, the type has. Clear is the intent, but documentation would help future padawans understand the way.
Add a brief comment explaining the mapping's purpose:
ReadDataHandle<MeasurementSimHitsMap> m_inputMeasurementSimHitsMap{ + // Maps measurements to their corresponding simulated hits for truth comparison this, "InputMeasurementSimHitsMap"};Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp (1)
51-51: Consider the performance implications, you must.While std::map automatic sorting brings, a price in performance it demands. For small track sizes, acceptable this is. But for larger collections, consider std::vector with manual sort, you should.
- std::map<double, Index> trackHitList; + std::vector<std::pair<double, Index>> trackHitList; // Later, after populating: + std::sort(trackHitList.begin(), trackHitList.end());Examples/Io/Root/src/TrackFinderNTupleWriter.cpp (2)
81-81: Consistent terminology in tree branches, you maintain!Yet, document the meaning of measurements in comments, you should. Help future padawans understand the data structure better, it will.
Consider adding a brief comment explaining what constitutes a measurement in this context.
Also applies to: 150-150
211-212: Correct use of measurement mapping, I observe!Yet, consider caching the measurement range size, we should, when used multiple times within the same scope.
-auto measurements = makeRange( - particleMeasurementsMap.equal_range(particle.particleId())); +auto measurements = makeRange( + particleMeasurementsMap.equal_range(particle.particleId())); +const auto measurementCount = measurements.size(); // Cache the size // Later usage -prtNumMeasurements = measurements.size(); +prtNumMeasurements = measurementCount;Also applies to: 225-227
Examples/Framework/include/ActsExamples/EventData/Index.hpp (1)
Line range hint
42-66: Elegant and powerful, the new implementation is!Template function, more flexible it has become. Yet a suggestion for improvement, I have:
Consider reserving space in the inverse map, reduce allocations it will:
InverseMultimap<value_t> inverse; +inverse.reserve(multimap.size()); #if BOOST_VERSION < 107800 for (const auto& i : unordered) { inverse.insert(i);Strong with BOOST_VERSION compatibility, this code is. Maintain backward support while embracing modern features, it does.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.cpp (2)
27-28: Update error message to reflect new terminology, you must.Hmmmm, inconsistent the error message is. Speaks of "hit-particles map" it does, while "particle-measurements map" we now use.
Apply this change, you should:
if (m_cfg.inputParticleMeasurementsMap.empty()) { - throw std::invalid_argument("Missing input hit-particles map collection"); + throw std::invalid_argument("Missing input particle-measurements map collection"); }
57-60: A suggestion for robustness, I have.Correct the changes are, but stronger they could become. Check for empty measurements before reserving space, wise it would be.
Consider this improvement, you should:
+ if (measurements.empty()) { + ACTS_VERBOSE(" - Skip particle with no measurements"); + continue; + } track.reserve(measurements.size()); for (const auto& measurement : measurements) { track.emplace_back(measurement.second); }Examples/Io/Root/src/RootTrackParameterWriter.cpp (2)
Line range hint
46-92: Additional error handling for TTree creation, suggest I must.Thorough your error checks are, but one weakness I sense. When creating TTree, protect against the dark side of nullptr, you should.
m_outputFile->cd(); - m_outputTree = new TTree(m_cfg.treeName.c_str(), m_cfg.treeName.c_str()); - if (m_outputTree == nullptr) { + if (!m_outputFile->IsOpen()) { + throw std::runtime_error("Output file not properly opened"); + } + m_outputTree = new TTree(m_cfg.treeName.c_str(), m_cfg.treeName.c_str()); + if (m_outputTree == nullptr) { throw std::bad_alloc(); + } + if (!m_outputTree->IsZombie()) { + throw std::runtime_error("TTree creation failed"); }
Line range hint
201-300: Extract complex calculations into named functions, wisdom suggests.Long and complex, this section is. Break down the force into smaller functions, we must. Magic numbers in error calculations, replace with named constants, you should.
+ static constexpr float NEGLIGIBLE_ERROR_THRESHOLD = 1e-6; + static constexpr float NAN_VALUE = std::numeric_limits<float>::quiet_NaN(); + + float calculatePull(float residual, float error) { + if (std::isnan(error) || std::abs(error) < NEGLIGIBLE_ERROR_THRESHOLD) { + return NAN_VALUE; + } + return residual / error; + } + // Replace existing pull calculations - auto getPull = [](float res, float err) -> float { - if (std::isnan(err) || std::abs(err) < 1e-6) { - return NaNfloat; - } - return res / err; - }; - m_pull_loc0 = getPull(m_res_loc0, m_err_loc0); - m_pull_loc1 = getPull(m_res_loc1, m_err_loc1); + m_pull_loc0 = calculatePull(m_res_loc0, m_err_loc0); + m_pull_loc1 = calculatePull(m_res_loc1, m_err_loc1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
Examples/Algorithms/Digitization/include/ActsExamples/Digitization/DigitizationAlgorithm.hpp(2 hunks)Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/SurfaceSortingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp(3 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.hpp(2 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.cpp(4 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.hpp(2 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.cpp(3 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.hpp(2 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthVertexFinder.hpp(2 hunks)Examples/Framework/include/ActsExamples/EventData/Index.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/Measurement.hpp(2 hunks)Examples/Framework/include/ActsExamples/EventData/SimHit.hpp(0 hunks)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSeedWriter.hpp(3 hunks)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvTrackWriter.hpp(4 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackParameterWriter.hpp(3 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp(2 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/SeedingPerformanceWriter.hpp(2 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderNTupleWriter.hpp(1 hunks)Examples/Io/Root/src/RootTrackParameterWriter.cpp(1 hunks)Examples/Io/Root/src/TrackFinderNTupleWriter.cpp(12 hunks)Examples/Python/python/acts/examples/simulation.py(1 hunks)Examples/Python/src/Digitization.cpp(1 hunks)Examples/Python/src/Output.cpp(1 hunks)Examples/Python/src/TruthTracking.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- Examples/Framework/include/ActsExamples/EventData/SimHit.hpp
🔇 Additional comments (43)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.hpp (1)
34-35: Approve the naming change, I do! Clearer intention, it shows.
Renamed from inputMeasurementParticlesMap to inputParticleMeasurementsMap, better reflects the relationship direction, it does. Consistent with the bidirectional mapping approach in the PR objectives, this change is.
Examples/Io/Root/include/ActsExamples/Io/Root/SeedingPerformanceWriter.hpp (1)
12-12: Approve the transition to Measurement header, I do!
Wise choice it is, to shift from SimHit to Measurement. Aligns with the force of consistency across the codebase, this change does.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.cpp (3)
28-36: Approve these changes, I do!
Simplified the constructor signature is, and clearer the error message has become. Wisdom in changing inputMeasurementParticlesMap to inputParticleMeasurementsMap, I sense. More intuitive the direction of mapping now is, hmm.
65-65: Consistent with the Force, this change is!
Aligned with the new naming convention, the initialization remains.
116-123: Strong with the Force, these changes are!
Clear and purposeful, the measurement handling has become. The way of the Jedi, this is.
Examples/Python/src/TruthTracking.cpp (1)
43-43: Approve the parameter renaming in TruthTrackFinder, I do.
More intuitive, the new parameter name is. From particle to measurements, the relationship flows, hmm. Consistent with the natural order of things, this change is.
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSeedWriter.hpp (3)
11-11: Wise addition of measurement header, I sense.
Properly aligned with the force of measurement handling, this include is. A natural consequence of the type changes below, it appears to be.
99-99: Removed the trailing comment has been. Clean and clear, the code becomes.
A small change, yet to readability it contributes. The force of clean code, strong it is.
80-83: Changed from hits to measurements, the data handles have.
Hmm, yes. Consistent with the PR's purpose, these changes are. From HitParticlesMap to MeasurementParticlesMap and from HitSimHitsMap to MeasurementSimHitsMap, the transformation flows. But verify the impact across the codebase, we must.
✅ Verification successful
Completed, the verification has been. Clear, the path forward is.
Through the Force, searched the entire codebase I have. The changes from HitParticlesMap and HitSimHitsMap to MeasurementParticlesMap and MeasurementSimHitsMap already propagated throughout the codebase they are. No remaining references to the old types found were. The new types consistently used they are, in:
- Digitization algorithm outputs them
- CSV, ROOT and EDM4hep I/O components handle them
- Truth tracking algorithms consume them
- Track fitting components utilize them
Harmonious, the change is. In balance with the rest of the codebase it stands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to old types that might need updating
echo "Search for old type references we shall..."
rg "HitParticlesMap|HitSimHitsMap" --type cpp
echo "And new type usage patterns we must find..."
rg "MeasurementParticlesMap|MeasurementSimHitsMap" --type cpp -A 2
Length of output: 35986
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvTrackWriter.hpp (2)
14-14: Hmmmm, wise change in the Force, I sense.
Aligned with the PR's objectives, this header change is. From SimHit to Measurement, the path leads. Consistent with the broader refactoring effort, this modification is.
83-84: Changed the type has, but verify its usage we must.
From HitParticlesMap to MeasurementParticlesMap, the transition is made. Ensure all consumers of this handle, updated they are.
✅ Verification successful
Verified, the changes are. Safe, the transition appears to be.
Through the Force, I sense that all consumers of this handle have already embraced MeasurementParticlesMap they have. No remaining references to HitParticlesMap exist, and all usages of m_inputMeasurementParticlesMap properly initialized and accessed they are.
The following files, updated correctly they are:
- Examples/Io/Root/src/SeedingPerformanceWriter.cpp
- Examples/Io/Root/src/RootTrackParameterWriter.cpp
- Examples/Io/Csv/src/CsvSeedWriter.cpp
- Examples/Io/Csv/src/CsvTrackWriter.cpp
- Examples/Io/EDM4hep/src/EDM4hepMultiTrajectoryWriter.cpp
- Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.cpp
- Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthVertexFinder.cpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to HitParticlesMap
rg "HitParticlesMap" --type cpp
# Search for usage of m_inputMeasurementParticlesMap
rg "m_inputMeasurementParticlesMap" --type cpp -A 5
Length of output: 12903
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackParameterWriter.hpp (2)
12-12: Approve the header change, I do.
Wise choice it is, to include Measurement.hpp. Aligns with the greater mission of measurement-based refactoring, it does.
88-91: Transition from Hit to Measurement maps, I sense.
Changed the data handles have been, from Hit-based to Measurement-based mappings. Consistent with the force of refactoring, this is. But verify the impact on the rest of the codebase, we must.
Run this verification script, we shall:
✅ Verification successful
Completed the transition to Measurement maps, the codebase has.
Through the Force, I sense that all references to old Hit-based maps have vanished, they have. The search results show only the new Measurement-based mappings across the entire codebase:
- MeasurementParticlesMap
- MeasurementSimHitsMap
No remnants of the old HitParticlesMap or HitSimHitsMap remain, indicating a complete and consistent transition throughout the codebase. The change aligns with the greater refactoring effort, it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to old hit-based maps that might need updating
echo "Search for old HitParticlesMap usage we shall..."
rg "HitParticlesMap" -l
echo "And for HitSimHitsMap references, look we must..."
rg "HitSimHitsMap" -l
echo "Check for consistency in measurement map usage, we will..."
rg "MeasurementParticlesMap|MeasurementSimHitsMap" -A 2
Length of output: 47577
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp (1)
12-12: Approve the transition to measurement-based approach, I do. Verify the impact, we must.
Changed from Index.hpp to Measurement.hpp, the header has. Aligns with our journey to measurement-centric ways, it does.
✅ Verification successful
Safe, the header change is. Proceed with confidence, we shall.
Revealed by the Force, it has been, that Measurement.hpp provides all necessary Index definitions, hmm. Contains it does the essential types and mappings:
- Index and SubspaceIndex types it defines
- MeasurementSimHitsMap and related mappings it provides
- No direct dependency on Index.hpp in Measurement.hpp exists
A balanced transition this is, maintaining harmony in the codebase while moving to measurement-centric ways, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Index.hpp includes that might need updating
# and verify no direct Index.hpp dependencies exist
# Search for any remaining Index.hpp includes
echo "Searching for remaining Index.hpp includes..."
rg -l "Index\.hpp"
# Check if Measurement.hpp provides all necessary symbols
echo "Checking for potential missing symbols..."
rg -l "class.*Index|struct.*Index|using.*Index" | grep -v "Measurement.hpp"
Length of output: 6235
Script:
#!/bin/bash
# Check if Measurement.hpp provides necessary Index-related definitions
echo "Checking Measurement.hpp for Index definitions..."
rg "class.*Index|struct.*Index|using.*Index|typedef.*Index" Examples/Framework/include/ActsExamples/EventData/Measurement.hpp
# Check if RootTrackStatesWriter.hpp uses any Index-specific types
echo "Checking RootTrackStatesWriter.hpp for Index usage..."
rg "Index" Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp
# Check if Index.hpp is still included indirectly through other headers
echo "Checking for indirect Index.hpp includes..."
rg -l "include.*Index\.hpp" Examples/Framework/include/ActsExamples/EventData/Measurement.hpp
Length of output: 1288
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthVertexFinder.hpp (2)
12-12: Wise addition of Measurement.hpp, I sense!
Align with the force of refactoring, this change does. From hits to measurements, the path leads us.
56-57: Changed the type to MeasurementParticlesMap, we have. Hmmmm.
A good change this is, yes. From the old ways of HitParticlesMap to the new path of MeasurementParticlesMap, we have evolved. Consistent with our quest to store relationships in both directions, this change is.
Run this verification, we must:
✅ Verification successful
Complete, the transition to MeasurementParticlesMap is. Hmmmm.
Found the definition in Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, I have:
using MeasurementParticlesMap = IndexMultimap<SimBarcode>;No traces of the old HitParticlesMap remain in the codebase, there do. Already completed, this migration has been. Consistent usage of MeasurementParticlesMap throughout the repository, I observe. In Python scripts, C++ files, and configuration parameters, the new way is followed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other files that might need similar type updates
rg "HitParticlesMap" --type cpp
Length of output: 31
Script:
#!/bin/bash
# Search with ripgrep in all files, not just cpp
rg "HitParticlesMap"
# Also search for the new type to understand the transition
rg "MeasurementParticlesMap"
# Use ast-grep to find type declarations and aliases
ast-grep --pattern 'using $_ = HitParticlesMap'
ast-grep --pattern 'using $_ = MeasurementParticlesMap'
ast-grep --pattern 'typedef $_ HitParticlesMap'
ast-grep --pattern 'typedef $_ MeasurementParticlesMap'
Length of output: 11399
Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp (4)
21-23: Simplified the constructor declaration is, hmmmm.
Removed namespace qualifiers, cleaner the code becomes. Validation checks remain strong with the Force, they do.
43-44: Consistent with the Force, this change is.
Removed namespace qualification, yet const correctness preserved it has. Wise decision, this is.
81-81: Complete the task does, return success it must.
Proper completion signaled it has. In balance with the Force, this return statement is.
Line range hint 43-81: Verify the consistency across the codebase, we must.
In other files, the measurement maps usage check we should. Connected through the Force, all components are.
✅ Verification successful
Consistent usage of measurement maps across the codebase, I confirm.
Through the Force, searched the codebase I have. Clear patterns emerge:
- In
Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, defined correctly theMeasurementSimHitsMaptype is - Consistent initialization and error handling across components like
DigitizationAlgorithm,RootTrackStatesWriter, and others, observe I do - Similar pattern of map usage for time-based sorting, unique to this algorithm it is
No disturbance in the Force I sense. The measurement map implementation follows the ways of the Jedi Council's standards throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related measurement map usage
echo "Searching for measurement map usage patterns..."
rg "MeasurementSimHitsMap|measurement.*map" --type cpp
# Check for similar sorting implementations
echo "Checking for similar sorting implementations..."
ast-grep --pattern 'std::map<double, $_> $_ = $_'
Length of output: 13300
Examples/Io/Root/src/TrackFinderNTupleWriter.cpp (4)
12-12: Hmmmm, proper include this is!
Aligned with measurement-centric approach, this change is. Clear dependency on measurement data structures, it shows.
101-103: Strong with error handling, this code is!
Clear and specific, the validation message is. Proper guard against missing data, it provides.
267-268: Clean and consistent, the implementation remains!
Removed redundant namespace qualifiers, you have. Pleased with this simplification, I am.
Also applies to: 272-272, 274-275, 285-285, 290-293
39-39: Consistent naming and type changes, I sense!
From hits to measurements, the transition flows smoothly. But verify the impact on dependent systems, we must.
Also applies to: 92-92, 114-114
✅ Verification successful
Successful transition to ParticleMeasurementsMap, I observe
Searched the entire codebase, I have. No traces of the old HitParticlesMap remain, while ParticleMeasurementsMap consistently used across the codebase, it is. A clean migration, this appears to be.
- In Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, properly defined the type is
- In Digitization, TruthTracking, and Python bindings, consistently used it is
- In your file, correctly implemented the changes are
Hmmmm, strong with the Force, this change is. No further verification needed, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old type
rg "HitParticlesMap"
# Search for consistency in measurement map usage
rg "ParticleMeasurementsMap"
Length of output: 4199
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/SurfaceSortingAlgorithm.hpp (1)
49-51: Hmmmm, wise this change is, but verify we must!
Changed from HitSimHitsMap to MeasurementSimHitsMap, the data handle has been. Consistent with the measurement-centric approach, this modification is. Yet verify the impact on downstream algorithms, we should.
✅ Verification successful
Verified, the change is. No traces of HitSimHitsMap remain, hmm.
Through the Force, searched the entire codebase I have. Only MeasurementSimHitsMap exists now, consistently used across all files it is. The old HitSimHitsMap, vanished completely it has. In harmony with the measurement-centric approach, this change flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to HitSimHitsMap
rg "HitSimHitsMap"
# Find all files using MeasurementSimHitsMap to ensure consistent usage
rg "MeasurementSimHitsMap"
Length of output: 10526
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.hpp (2)
12-12: Changed the include path has been, hmmmm.
From SimHit.hpp to Measurement.hpp, the path now leads. A step towards measurement-centric architecture, this is.
57-59: The force of consistency, strong with this one it is!
From HitParticlesMap to MeasurementParticlesMap, evolved the type has. Aligns with the broader refactoring vision, this change does. Yet careful we must be, for impact on existing algorithms, verify we should.
✅ Verification successful
Hmm, clear the path forward has become!
Through the Force, searched the entire codebase I have. No traces of the old "HitParticlesMap" remain, I see. Consistently used throughout the repository, "MeasurementParticlesMap" is. A complete transition, this represents.
In the files that use it:
- Properly defined in
Examples/Framework/include/ActsExamples/EventData/Measurement.hpp - Consistently used across Python scripts, CSV handlers, and tracking algorithms
- Properly initialized and handled in all implementations
Successful and complete, this refactoring is. No further verification needed, as balance to the Force, this change brings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to HitParticlesMap
rg "HitParticlesMap"
# Verify consistent usage of MeasurementParticlesMap
rg "MeasurementParticlesMap"
Length of output: 11183
Examples/Framework/include/ActsExamples/EventData/Index.hpp (1)
33-40: Documentation, the path to wisdom it is!
Clear and concise, the new documentation is. Explains the purpose of InverseMultimap well, it does. A valuable addition to our sacred texts, this is.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.hpp (2)
33-34: Approve the renaming of configuration parameter, I do! More clarity it brings!
The new name inputParticleMeasurementsMap better describes the relationship direction from particles to measurements. Consistent with the PR's objective of establishing clear relationships between particles and measurements, this change is.
73-74: Wise choice of InverseMultimap, it is! Better suited for the task at hand!
The change from HitParticlesMap to InverseMultimap<SimBarcode> aligns well with the mapping requirements. More flexible and efficient for storing many-to-many relationships between particles and measurements, this data structure is.
Examples/Python/src/Digitization.cpp (1)
58-59: Python bindings, properly exposed they are! Harmony between C++ and Python, maintained it is!
The addition of outputParticleMeasurementsMap and outputSimHitMeasurementsMap to the Python bindings ensures consistent configuration capabilities across both C++ and Python interfaces.
Examples/Algorithms/Digitization/include/ActsExamples/Digitization/DigitizationAlgorithm.hpp (2)
51-54: New configuration options, well-named they are! Clear purpose they serve!
The addition of outputParticleMeasurementsMap and outputSimHitMeasurementsMap completes the bidirectional mapping capability. Store relationships in both directions, we now can!
147-151: WriteDataHandle declarations, properly structured they are! Consistent with the design patterns!
The new data handles use appropriate types:
InverseMultimap<SimBarcode>for particle-to-measurements mappingInverseMultimap<Index>for simhit-to-measurements mapping
Descriptive labels in initialization, helpful for debugging they are!
Examples/Framework/include/ActsExamples/EventData/Measurement.hpp (1)
535-540: Well-structured type aliases, you have created!
Clear and organized, these type definitions are. The path to clean code, you follow:
- MeasurementSimHitsMap and MeasurementParticlesMap for forward mappings
- SimHitMeasurementsMap and ParticleMeasurementsMap for inverse mappings
Strong foundation for bidirectional relationships, this provides.
Examples/Python/python/acts/examples/simulation.py (1)
792-793: Python bindings, properly extended they are!
Harmony between C++ and Python, you maintain. Clear naming and consistent integration, I observe.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.cpp (2)
42-42: Approve these changes, I do.
Consistent with the new measurement-focused approach, these changes are. Clear and precise, the variable naming has become. Strong with the Force, this refactoring is.
Also applies to: 51-54
35-35: Verify consistency across the codebase, we must.
Hmmmm, ensure we must that all usages of particle measurements mapping follow this new pattern.
Also applies to: 42-42
✅ Verification successful
Consistent with the Force, these changes are.
Examined the codebase thoroughly, I have. The pattern of inputParticleMeasurementsMap usage, consistent it remains across all files. In TruthTrackFinder, TruthSeedingAlgorithm, and TrackFinderNTupleWriter, the same initialization pattern follows:
- Initialize with configuration they do
- Proper error handling they maintain
- Consistent naming conventions they follow
No disturbance in the Force I sense, as all implementations aligned they are. The old terms "inputMeasurementParticlesMap" and "hitParticlesMap" appear only in legacy files that require no changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old terminology
rg -l "inputMeasurementParticlesMap|hitParticlesMap" --type cpp
# Search for similar mapping patterns to ensure consistency
ast-grep --pattern 'inputParticleMeasurementsMap'
Length of output: 2056
Script:
#!/bin/bash
# Let us examine the context of these occurrences more deeply
rg "inputParticleMeasurementsMap" -B2 -A2 --type cpp Examples/Algorithms/TruthTracking/
# And see the full context of other implementations
rg "inputParticleMeasurementsMap" -B2 -A2 Examples/Io/Root/src/TrackFinderNTupleWriter.cpp
# Search for initialize pattern specifically
ast-grep --pattern '$_.initialize($_)'
Length of output: 7858
Examples/Io/Root/src/RootTrackParameterWriter.cpp (2)
43-44: Approve the base class modification, I do!
Wise choice it is, to use WriterT. Better type safety and consistency with modern practices, this brings. Hmmmm.
Line range hint 1-350: Verify the impact on dependent systems, we must.
Changed from TrackParameterWriter to WriterT, ripples through the force may create. Check other writers that depend on this base class, we should.
✅ Verification successful
Safe, the changes are. No disturbance in the Force, I sense.
Through the Force, seen I have that both RootTrackParameterWriter and CsvTrackParameterWriter already use WriterT as their foundation. A natural evolution of the codebase, this change is. No breaking changes to dependent systems, there are.
- The Python bindings in Examples/Python/src/Output.cpp, prepared for this pattern, they already were
- The CSV writer in Examples/Io/Csv, aligned with this design, it remains
- The configuration structures, unchanged they stay, hmmmm
The Force is strong with this refactoring. Clean and consistent with the measurement-centric approach, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other writers that might be affected by base class change
echo "Searching for other writers potentially affected by base class change..."
rg -l "TrackParameterWriter" --type cpp
# Search for direct usage of the class
echo "Checking direct usage of RootTrackParameterWriter..."
rg -l "RootTrackParameterWriter" --type cpp --type hpp
Length of output: 668
Script:
#!/bin/bash
# Search with proper file type flags and check implementations
echo "=== Searching RootTrackParameterWriter usage ==="
rg -l "RootTrackParameterWriter"
echo -e "\n=== Checking base class inheritance patterns ==="
ast-grep --pattern 'class $_ : public WriterT<$_> { $$$ }'
echo -e "\n=== Examining CsvTrackParameterWriter implementation ==="
rg -l "class CsvTrackParameterWriter" --type cpp --type-add "hpp:*.hpp"
echo -e "\n=== Looking for TrackParameterWriter configurations ==="
rg "TrackParameterWriter::Config" -A 5
Length of output: 2285
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp (4)
61-68: Wise validation checks added, hmm yes!
Proper error handling for new configuration parameters, I see. Consistent with existing pattern, these checks are. Clear error messages, they provide.
152-152: Changed to SimBarcode, the type has been.
More appropriate for simulation hit tracking, this type is. Aligns with the force of better relationship mapping, it does.
75-77: Initialize the new output handles, we must.
Proper initialization of new mapping handles, I observe. Follows the way of existing patterns, it does.
316-319: Bidirectional force flows through these mappings, it does.
Wise decision to use invertIndexMultimap, I sense. Both directions of relationships, we now maintain. Performance impact of map inversion, we should verify.
✅ Verification successful
Let us seek deeper understanding of these data structures, we must. Their implementation details, crucial they are, for performance judgment to make.
Hmm, elusive these implementation details are. Another approach, we shall try. The full content of Index.hpp, reveal our answers it might. If performance tests exist, find them we must.
Efficient and well-implemented, the invertIndexMultimap function is, hmmmm.
Clear now, the implementation becomes. Using boost::container::flat_multimap, optimal performance it achieves. Linear complexity for inversion it has, with pre-allocation of memory through reserve(). Additional optimization through ordered_range_t in newer Boost versions, I see.
The Force is strong with this implementation:
- O(n) complexity for the inversion operation
- Memory pre-allocation, preventing reallocations it does
- Sorted insertion in newer Boost versions, additional performance it brings
- flat_multimap underneath, cache-friendly it remains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of invertIndexMultimap for performance considerations
# Search for invertIndexMultimap implementation and usage
rg -A 5 "invertIndexMultimap"
# Look for any existing performance tests
fd -e cpp -e hpp -e txt "test.*performance" -X rg "invertIndexMultimap"
Length of output: 1997
Script:
#!/bin/bash
# Investigate the implementation details of IndexMultimap and InverseMultimap
# Check the full implementation of Index.hpp
ast-grep --pattern 'template$$$class IndexMultimap$$$'
# Look for the actual data structure and complexity hints
ast-grep --pattern 'struct InverseMultimap$$$'
# Check if there are any comments about performance or complexity
rg -B 2 -A 5 "class (Index|Inverse)Multimap"
Length of output: 152
Script:
#!/bin/bash
# Search for Index.hpp content more broadly
cat Examples/Framework/include/ActsExamples/EventData/Index.hpp
# Also check for any performance-related tests
rg -l "IndexMultimap.*test"
rg -l "InverseMultimap.*test"
Length of output: 2476
Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderNTupleWriter.hpp
Show resolved
Hide resolved
…nd/acts into ex-digi-particle-measurement-maps
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp (2)
313-318: Wise ordering of operations, this is!Before moving original maps, inverted they are. Prevent use-after-move issues, this sequence does. Clear comment explains timing of inversion, it does.
However, consider we should, adding error handling for invertIndexMultimap operations.
- m_outputParticleMeasurementsMap( - ctx, invertIndexMultimap(measurementParticlesMap)); - m_outputSimHitMeasurementsMap(ctx, - invertIndexMultimap(measurementSimHitsMap)); + auto invertedParticleMap = invertIndexMultimap(measurementParticlesMap); + auto invertedSimHitMap = invertIndexMultimap(measurementSimHitsMap); + + if (invertedParticleMap.size() != measurementParticlesMap.size() || + invertedSimHitMap.size() != measurementSimHitsMap.size()) { + ACTS_WARNING("Possible data loss during map inversion detected"); + } + + m_outputParticleMeasurementsMap(ctx, std::move(invertedParticleMap)); + m_outputSimHitMeasurementsMap(ctx, std::move(invertedSimHitMap));
313-318: Consider performance impact of map inversion, we should!For large datasets, performance impact of map inversion significant could be. Consider caching inverted maps or implementing lazy inversion, we might.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp(3 hunks)
🔇 Additional comments (3)
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp (3)
61-68: Proper validation checks, these are!
Wisely added, the validation for new mapping configurations is. Consistent with existing error handling pattern, it remains. Clear error messages, it provides.
75-77: Initialization of new output handles, correct it is!
Within the clusterization block, properly placed these initializations are. With existing pattern, they align.
152-152: Type change to SimBarcode, verify we must!
Changed from ActsFatras::Barcode to SimBarcode, the type has been. Ensure proper definition and import of SimBarcode, we should.
✅ Verification successful
Verified, the type change is. Properly defined and imported, SimBarcode appears to be!
Found in Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp, SimBarcode is:
using SimBarcode = ::ActsFatras::Barcode;Throughout the codebase, extensively used it is. In digitization, tracking, and simulation components, properly integrated it appears to be. A valid and well-established type in the Acts framework, SimBarcode is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SimBarcode definition and usage
rg -A 2 "SimBarcode"
Length of output: 21002
Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp
Outdated
Show resolved
Hide resolved
|
…in Examples (#3947) This clears out an oversight in a previous PR #3742 where I replaced numbers of measurements with number of hits. This number can be different and should not be confused. I add an optional input for the particle to measurement map which is then queried if present and used for the number of measurement cuts. blocked by: - #3944 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced particle selection with new configuration options for minimum and maximum hit counts. - Updated particle selection criteria to focus on hit counts instead of measurement counts. - **Bug Fixes** - Improved validation checks for input measurements to ensure proper initialization. - **Documentation** - Updated comments and documentation to reflect changes in parameter naming from "measurements" to "hits." - **Tests** - Adjusted test cases to align with the new parameter naming conventions. - **Chores** - Minor code formatting and organization improvements throughout various scripts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
In #3944 we found out that this algorithm is not needed anymore <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced configurability for machine learning-based seed filtering with the addition of an ONNX model parameter. - Streamlined seeding and track fitting algorithms for improved modularity. - **Bug Fixes** - Removed unnecessary complexity by eliminating the `directNavigation` parameter from multiple functions. - **Documentation** - Updated test suite to improve clarity and maintainability. - **Chores** - Removed obsolete files and references related to the `SurfaceSortingAlgorithm` class to streamline the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
We have a lot of cases where we need particle/simhit -> measurement relations but on the whiteboard we only have the opposite. I believe it makes more sense to store the relations in both directions on the whiteboard instead of inverting in the downstream algorithms.
I use this new map in a couple of occasions and cleaned out wrongly defined typedefs apart from my usual Examples cleanup.
Summary by CodeRabbit
outputParticleMeasurementsMapandoutputSimHitMeasurementsMap.