fix: Distinguish between hits and measurements in ParticleSelector in Examples
#3947
Conversation
## Walkthrough
Enhance the validation logic and configurability, the changes to the `ParticleSelector` class across multiple files do. Conditional initialization of a new member variable for input particle measurements, the constructor in `ParticleSelector.cpp` now includes. Validation checks ensure proper initialization of measurement maps, and new hit count parameters are introduced. Updates to `simulation.py` accommodate these changes in the `ParticleSelectorConfig`, while various scripts and tests reflect updates to parameter names and configurations related to hits instead of measurements.
## Changes
| File | Change Summary |
|------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp` | Constructor updates for conditional initialization of `m_inputParticleMeasurementsMap`. New validation checks and logic for counting invalid particles. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp` | Added member variables `inputParticleMeasurementsMap`, `hitsMin`, and `hitsMax` to `Config`. Updated comments for clarity. |
| `Examples/Python/python/acts/examples/simulation.py` | Enhanced `ParticleSelectorConfig` with `hits` field. Updated functions to utilize new hit parameters. |
| `Examples/Python/src/TruthTracking.cpp` | Renamed `inputMeasurementParticlesMap` to `inputParticleMeasurementsMap`. Expanded `Config` structure with `hitsMin` and `hitsMax`. |
| `CI/physmon/workflows/physmon_trackfinding_1muon.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `CI/physmon/workflows/physmon_trackfinding_4muon_50vertices.py` | Updated parameter name from `measurements` to `hits` in `addFatras`. |
| `CI/physmon/workflows/physmon_trackfinding_ttbar_pu200.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Python/tests/test_examples.py` | Updated parameter name from `measurements` to `hits` in `test_itk_seeding`. |
| `Examples/Scripts/Optimization/ckf.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/ckf_tracks.py` | Updated parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/full_chain_itk.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/full_chain_itk_Gbts.py` | Updated parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/full_chain_odd.py` | Changed parameter name from `measurements` to `hits` in both `addGeant4` and `addFatras`. |
| `Examples/Scripts/Python/full_chain_odd_LRT.py` | Updated parameter name from `measurements` to `hits` in both `addGeant4` and `addFatras`. |
| `Examples/Scripts/Python/full_chain_test.py` | Modified `postSelectParticles` configuration to change `measurements` to `hits`. Added error handling in `strToRange`. |
| `Examples/Scripts/Python/hashing_seeding.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/seeding.py` | Updated parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/truth_tracking_gsf.py` | Changed parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/truth_tracking_gx2f.py` | Updated parameter name from `measurements` to `hits` in `addFatras`. |
| `Examples/Scripts/Python/truth_tracking_kalman.py` | Changed parameter name from `measurements` to `hits` in `ParticleSelectorConfig`. |
## Possibly related PRs
- **#3812**: Related to enhancements in `simulation.py` for particle selection criteria.
- **#3836**: Aligns with updates to the `HitSelector` class for refining selection criteria.
- **#3907**: Focuses on validation logic improvements similar to those in `ParticleSelector`.
- **#3929**: Relevant to hit count functionality enhancements in `RootAthenaDumpReader`.
## Suggested labels
`Track Finding`, `Event Data Model`
## Suggested reviewers
- andiwand
> In the realm of particles, we play,
> With hits and measures, come what may.
> A selector refined, its logic anew,
> In code we trust, with each line we drew.
> From measurements to hits, the journey unfolds,
> In the dance of the data, a story retold. ✨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 (
|
![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 (19)
Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderNTupleWriter.hpp (1)
33-34: Hmmmm, wise this name change is, approve it I do!Clear distinction between particles and their measurements, this brings. Consistent with the force of proper terminology, it now flows. A path to better code clarity, this change creates.
Remember you must, that throughout the codebase, this naming pattern maintain we should. Consistency in terminology, crucial it is, for understanding the ways of the code.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.cpp (2)
27-28: Update error message to reflect new terminology, you must.Hmmmm. Inconsistent with new naming convention, the error message is. "Missing input hit-particles map collection" it says, when "measurement-particles map" it should be.
if (m_cfg.inputParticleMeasurementsMap.empty()) { - throw std::invalid_argument("Missing input hit-particles map collection"); + throw std::invalid_argument("Missing input measurement-particles map collection"); }
68-69: Strong and clean, this code has become.Pleased with the overall changes, I am. Consistent naming throughout the file, there is. Good practices maintained, they were. A path to better code clarity, these changes have created.
Remember to update related test files to reflect these naming changes, you must. Documentation updates, consider also.
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp (1)
Line range hint
12-108: Architectural wisdom, share I must.Strong in the force, this refactoring is. Yet, validate these points, you should:
- Clear separation between measurement and hit concepts in documentation, maintain
- Impact on downstream consumers of this writer, consider
- Migration guide for users of old API, provide
Through proper abstraction and documentation, balance to the force it brings.
Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp (1)
Line range hint
43-81: Optimize this code, we can. Yes, hrrmmm.Wise the use of std::map for sorting is, but multiple traversals, unnecessary they are. Reserve space for sortedProtoTrack after map size known, we should.
Apply this improvement, you should:
trackHitList.clear(); if (protoTrack.empty()) { continue; } for (const auto hit : protoTrack) { const auto simHitIndex = simHitsMap.find(hit)->second; auto simHit = simHits.nth(simHitIndex); auto simHitTime = simHit->time(); trackHitList.insert(std::make_pair(simHitTime, hit)); } + sortedProtoTrack.reserve(trackHitList.size()); /// Map will now be sorted by truth hit time for (auto const& [time, hit] : trackHitList) { sortedProtoTrack.emplace_back(hit); }Examples/Io/Root/src/TrackFinderNTupleWriter.cpp (1)
Line range hint
1-294: A suggestion for documentation, have I.Document the distinction between hits and measurements in class comments, helpful it would be. Future padawans, understand better they will.
Add this documentation at the class level:
+ // This class writes track finding results to a ROOT NTuple. + // Note: This implementation distinguishes between hits (raw detector responses) + // and measurements (calibrated detector measurements). The number of measurements + // may differ from the number of hits for a particle. class TrackFinderNTupleWriter {Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvTrackWriter.hpp (1)
Line range hint
92-97: Hmmmm, potential improvement in TrackInfo, I see.Consider documenting the
measurementsIDmember, you should. Clear documentation, the path to understanding is.Apply this change, you must:
std::vector<std::uint64_t> measurementsID; + /// Vector of measurement identifiers associated with this trackExamples/Io/Root/src/RootTrackParameterWriter.cpp (2)
Line range hint
47-63: Strong with error handling, this code is.Thorough validation of input collections, I sense. Particularly wise, the explicit checks for measurement maps are. Clear error messages, help future Padawans they will.
Consider logging warnings before throwing exceptions, helpful for debugging it would be.
Line range hint
249-269: Handle truth matching with great care, this code does.Robust implementation for particle identification and truth matching, I observe. Careful handling of edge cases with NaN values, impressive it is. Use of measurement maps aligns with the force... err, I mean, with the PR objectives of distinguishing hits from measurements.
Consider adding comments explaining the magic number
1e-6in error comparison:- if (std::isnan(err) || std::abs(err) < 1e-6) { + // Avoid division by very small errors that could lead to unstable pull values + if (std::isnan(err) || std::abs(err) < 1e-6) {Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (4)
33-42: Clearer error message needed, hmm yes.More explicit about the relationship between measurements and map, the error message should be. Help users understand better, it would.
- "Measurement-based cuts require the inputMeasurementParticlesMap"); + "Measurement-based cuts (min: " + std::to_string(m_cfg.measurementsMin) + + ", max: " + std::to_string(m_cfg.measurementsMax) + + ") require the inputMeasurementParticlesMap to be initialized");
60-61: Additional context in debug logging, we should add.Distinguish between hits and measurements more clearly in logs, we must. Help future debugging, it will.
- ACTS_DEBUG("selection particle hits [" << m_cfg.hitsMin << "," - << m_cfg.hitsMax << ")"); + ACTS_DEBUG("selection particle hits (from truth) [" << m_cfg.hitsMin << "," + << m_cfg.hitsMax << ")");
79-83: Document the purpose of empty map, we should.Clever the static empty map is, but its purpose unclear to young padawans it might be.
+ // Provide an empty map as fallback when measurement mapping is not configured + // This allows uniform handling of measurement counting logic const static InverseMultimap<SimBarcode> emptyMeasurementParticlesMap;
109-116: Optimize validation order for performance, we can.Good the separation between hits and measurements is, but performance improve we might. Move the measurement count check after other lightweight validations, we should.
- const bool validHitCount = - within(p.numberOfHits(), m_cfg.hitsMin, m_cfg.hitsMax); - nInvalidHitCount += static_cast<std::size_t>(!validHitCount); - - const std::size_t measurementCount = - inputMeasurementParticlesMap.count(p.particleId()); - const bool validMeasurementCount = - within(measurementCount, m_cfg.measurementsMin, m_cfg.measurementsMax); + // Check lightweight validations first + const bool validHitCount = + within(p.numberOfHits(), m_cfg.hitsMin, m_cfg.hitsMax); + nInvalidHitCount += static_cast<std::size_t>(!validHitCount); + + // Only check measurements if other criteria pass + if (validPdg && validCharge && validSecondary && + validPrimaryVertexId && validHitCount) { + const std::size_t measurementCount = + inputMeasurementParticlesMap.count(p.particleId()); + const bool validMeasurementCount = + within(measurementCount, m_cfg.measurementsMin, m_cfg.measurementsMax); + nInvalidMeasurementCount += + static_cast<std::size_t>(!validMeasurementCount); + }Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.cpp (2)
126-128: Wisdom in validation, there is.Clear warning message added you have, when insufficient measurements found are. But more specific the message could be.
- ACTS_WARNING("Particle " << particle << " has less than 3 measurements"); + ACTS_WARNING("Particle " << particle << " has " << track.size() + << " measurements, minimum of 3 required");
134-137: Improved clarity in comments, I sense.Updated comments to reflect measurements instead of hits, maintaining consistency they do. But a potential performance improvement, I see.
Consider reserving space for spacePointsOnTrack based on track size:
std::vector<const SimSpacePoint*> spacePointsOnTrack; spacePointsOnTrack.reserve(track.size()); - // Loop over the measurement index on the proto track to find the space - // points + // Loop over the measurement indices to find corresponding space points for (const auto& measurementIndex : track) {Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSeedWriter.hpp (1)
98-99: Comment cleanup, noticed I have.Removed unnecessary struct name from comment, cleaner the code becomes. Small detail, but attention to cleanliness, it shows.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp (1)
91-92: Handle the measurements map properly, you do!Consistent with ACTS framework patterns, this implementation is. Consider documenting the mapping structure in class documentation, you should.
Examples/Framework/include/ActsExamples/EventData/Measurement.hpp (1)
20-20: Clear type system, established we have.Strong typing for measurement mappings, introduced it is:
- MeasurementSimHitsMap and MeasurementParticlesMap for forward mappings
- SimHitMeasurementsMap and ParticleMeasurementsMap for inverse mappings
Clarity in relationships between measurements, hits, and particles, this brings.
Consider documenting these type aliases with Doxygen comments, help future Padawans it will.
Also applies to: 535-540
Examples/Python/python/acts/examples/simulation.py (1)
795-796: Strong with the Force, these new mappings are!Clear tracking of relationships between particles, measurements, and simhits, this provides. Yet, document the purpose of these mappings in comments, we should.
Add documentation like this, you could:
outputMeasurementSimHitsMap="measurement_simhits_map", +# Map between particles and their measurements outputParticleMeasurementsMap="particle_measurements_map", +# Map between simulation hits and their corresponding measurements outputSimHitMeasurementsMap="simhit_measurements_map",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
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/ParticleSelector.cpp(7 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp(4 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(3 hunks)Examples/Python/src/Digitization.cpp(1 hunks)Examples/Python/src/Output.cpp(1 hunks)Examples/Python/src/TruthTracking.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- Examples/Framework/include/ActsExamples/EventData/SimHit.hpp
🔇 Additional comments (51)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.cpp (1)
51-54: Approve these changes, I do. Clear and consistent, the new implementation is.
Wise changes these are. From "hits" to "measurements" the terminology shifts, maintaining consistency throughout the code it does. The Force is strong with this refactoring.
Also applies to: 57-59
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthVertexFinder.hpp (2)
12-12: Wise addition of Measurement.hpp, this is!
From hits to measurements, our path leads. Clear separation between concepts, we now maintain.
56-58: Strong with the Force, this type change is!
From HitParticlesMap to MeasurementParticlesMap, evolved we have. Proper distinction between hits and measurements, this brings. Yet verify its usage throughout the codebase, we must.
✅ Verification successful
Consistent usage of MeasurementParticlesMap throughout the codebase, I see
Through the Force, searched the entire codebase I have. No traces of the old HitParticlesMap remain, and properly integrated the new MeasurementParticlesMap is. The change, harmonious with the codebase it is.
Key observations, these are:
- In Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, properly defined MeasurementParticlesMap as IndexMultimap it is
- Throughout digitization, truth tracking, and IO components, consistently used the new type is
- No remnants of HitParticlesMap in the codebase exist
The change, complete and correct it is. Safe to proceed, you are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of MeasurementParticlesMap across the codebase
# Search for any remaining references to HitParticlesMap
echo "=== Searching for remaining HitParticlesMap references ==="
rg "HitParticlesMap" -A 2
# Search for consistent usage of MeasurementParticlesMap
echo "=== Verifying MeasurementParticlesMap usage ==="
rg "MeasurementParticlesMap" -A 2
Length of output: 29475
Examples/Io/Root/include/ActsExamples/Io/Root/SeedingPerformanceWriter.hpp (2)
12-12: Wise choice, this header change is!
From SimHit to Measurement, the path leads. Aligned with our goals to distinguish between hits and measurements, this change is.
83-84: Hmmmm... Correct this change is, but verify we must!
Changed the type from HitParticlesMap to MeasurementParticlesMap, you have. Wise decision this is, matching our journey from hits to measurements. Yet, verify the consistency across the codebase, we should.
✅ Verification successful
Hmmmm... Complete transformation to MeasurementParticlesMap, I sense!
Through the Force, searched the entire codebase I have. No remaining instances of HitParticlesMap found were. Consistently used MeasurementParticlesMap is, across all files. The change in SeedingPerformanceWriter.hpp aligns with the rest of the codebase it does.
Paths where MeasurementParticlesMap now dwells:
- Framework definitions in Examples/Framework/include/ActsExamples/EventData/Measurement.hpp
- Writers and Readers in Examples/Io/Root, Examples/Io/Csv, Examples/Io/EDM4hep
- Core algorithms in Examples/Algorithms/TruthTracking, Examples/Algorithms/Digitization
- Python bindings and configuration in Examples/Python
Correct this change is. Disturb the Force, it does not!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of MeasurementParticlesMap across the codebase
# Search for any remaining instances of HitParticlesMap
echo "Searching for old HitParticlesMap usage..."
rg "HitParticlesMap" -l
# Search for consistent usage of MeasurementParticlesMap
echo "Verifying MeasurementParticlesMap usage..."
rg "MeasurementParticlesMap" -A 2
Length of output: 29431
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackStatesWriter.hpp (2)
12-12: Wise choice, the header change is, hmmmm.
From Index.hpp to Measurement.hpp, the path leads. Align with the force of proper measurement handling, it does.
107-108: Changed from HitSimHitsMap to MeasurementSimHitsMap, the type has.
Proper distinction between hits and measurements, this change brings. Thread-safe through the mutex protection, it remains. Verify its usage throughout the codebase, we must.
✅ Verification successful
Complete, the migration to MeasurementSimHitsMap appears to be.
Searched through the entire codebase, I have. No instances of HitSimHitsMap remain, while MeasurementSimHitsMap consistently used throughout the codebase, it is. In Python bindings, IO operations, algorithms, and test cases, proper adoption observed, it has been.
Through the Force, see the following evidence, we do:
- Defined in
Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, the type MeasurementSimHitsMap is - Consistently used across CSV, Root, and EDM4hep IO components, it is
- In digitization, track fitting, and track finding algorithms, properly integrated, we find it
- Test cases updated accordingly, they have been
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of HitSimHitsMap to ensure complete migration
# Search for any remaining HitSimHitsMap usage
rg "HitSimHitsMap" --type cpp
# Search for MeasurementSimHitsMap usage to verify consistent adoption
rg "MeasurementSimHitsMap" --type cpp
Length of output: 8424
Examples/Python/src/Output.cpp (3)
201-201: Wise, this parameter rename is!
From inputMeasurementParticlesMap to inputParticleMeasurementsMap, changed the parameter name has been. Clearer distinction between measurements and particles, this brings. With the Force of consistent naming, stronger our codebase becomes!
201-201: Aligned with our mission, this change is!
With the PR objectives, harmony this brings. Between hits and measurements, clear distinction now exists. Strong with the Force, this change is!
201-201: Verify the consistency of renaming throughout the codebase, we must!
Search for other occurrences of similar mappings, we shall. Ensure consistent naming across the galaxy of our codebase, we must!
Examples/Algorithms/TrackFitting/src/SurfaceSortingAlgorithm.cpp (2)
Line range hint 21-42: Wise validation checks, you have added!
Strong with the Force, this constructor is. Proper validation for all inputs, it now has. Missing configurations, catch early we must.
Line range hint 29-31: Verify measurement mapping, we must.
Correct handling of measurement to sim hit mapping, crucial it is. Validate we must that inputMeasurementSimHitsMap properly maintains distinction between hits and measurements.
✅ Verification successful
Proper handling of measurement mapping, confirmed it is.
Through the Force, seen I have that MeasurementSimHitsMap consistently used across the codebase it is. In Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, defined as IndexMultimap<Index> the type is, and throughout IO components and algorithms, this mapping maintained properly it is.
Clear patterns observed:
- In digitization, output mapping created it is
- In readers, proper initialization and context handling observed it is
- In writers and algorithms, input mapping checked and validated it is
- Distinction between measurements and hits, preserved across all components it is
The Force tells me that proper distinction between hits and measurements, maintained it is. Consistent with PR objectives, this implementation stands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of measurement mapping to ensure consistency
rg "MeasurementSimHitsMap" --type cpp
Length of output: 8394
Examples/Io/Root/src/TrackFinderNTupleWriter.cpp (5)
39-39: Hmmmm, wise addition of particle measurements map, I sense.
Properly declared and initialized, the new data handle is. Clear separation between hits and measurements, this brings.
Also applies to: 92-92
101-103: Strong with validation, this code is.
Proper error checking for the particle-measurements map, essential it is. Missing configurations, catch early we must.
211-212: Refactored to use measurements instead of hits, the code has been.
A significant improvement in clarity, this change brings. Consistent with the PR objectives, it is. The force of proper terminology, strong it is.
Also applies to: 225-227
267-268: Simplified namespace usage, I observe.
Removed redundant namespace prefixes, cleaner the code becomes. Yet maintain functionality, it does.
Also applies to: 274-275, 277-278, 280-280, 285-285, 290-290
150-150: Changed from hits to measurements in tree branches, observe I do.
Renamed variables reflect true nature of data they hold. But verify the impact on existing analysis scripts, we must.
Also applies to: 245-245
✅ Verification successful
Safe to proceed with the change, it is. Local scope, the impact has.
Through the Force, seen I have that nhits branch exists only in this file, where the change occurs. Other nhits mentions in the file refer to different branches - particle_nhits_total and particle_nhits_on_track they are, untouched by this change they remain. No external analysis scripts dependent on this branch name, found we have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for analysis scripts that might depend on the old branch name
rg -g '*.{cpp,py,C}' -l 'nhits'
Length of output: 80
Script:
#!/bin/bash
# Search for the context around nhits usage to understand if it's just a declaration or used elsewhere
rg -g '*.{cpp,py,C}' 'nhits' -B 2 -A 2
# Also check if there are any other measurement-related branch names
rg -g '*.{cpp,py,C}' 'measurements.*Branch|Branch.*measurements'
Length of output: 1345
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvTrackWriter.hpp (3)
14-14: Wise change in the Force, I sense.
From SimHit to Measurement, the path leads. Alignment with the greater purpose of distinguishing measurements from hits, this change brings, hmmmm.
43-60: Clear documentation, the Config struct now has.
Well documented the configuration parameters are, young padawan. But verify the default values, we must.
✅ Verification successful
Let me search for consistency in other default values and any documentation about these parameters, hmm.
Consistent with the Force, these default values are.
Searched through the codebase, I have. Wisdom to share, I possess:
- The minimum number of measurements (7) aligned across multiple components it is, including core tracking and ambiguity resolution.
- The truth match probability threshold (0.5) and minimum pt (1 GeV) values, consistent with tracking performance scripts they are.
- Well-chosen defaults they appear to be, based on their widespread use in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Config structs with similar parameters
rg -A 5 "nMeasurementsMin\s*=\s*7" --type cpp
Length of output: 3716
Script:
#!/bin/bash
# Search for other default values of truthMatchProbMin and ptMin
rg -A 2 "truthMatchProbMin\s*=\s*0\.5|ptMin\s*=\s*1" --type cpp
# Search for any documentation about these parameters in markdown files
rg -i "nMeasurementsMin|truthMatchProbMin|ptMin.*track" --type md
Length of output: 893
80-83: Changed the type has been, from HitParticlesMap to MeasurementParticlesMap.
Consistent with our mission to separate hits from measurements, this change is. But careful we must be, to ensure all consumers of this map updated are.
✅ Verification successful
The Force guides me to search both the old and new types, as the transition from one to another, crucial it is to understand. More context we need, hmm.
Consistent with the change, all consumers are. Safe, this transition is.
Through the Force, seen I have that all consumers of this map type already use MeasurementParticlesMap. The change in CsvTrackWriter.hpp aligns with the existing usage across the codebase. No instances of the old HitParticlesMap remain, and all readers and writers properly handle MeasurementParticlesMap.
Key evidence from the search results:
- In Framework/EventData/Measurement.hpp, defined the type is:
using MeasurementParticlesMap = IndexMultimap<SimBarcode> - All consumers (writers, readers, algorithms) consistently use MeasurementParticlesMap in their configurations and handles
- No remnants of HitParticlesMap in the codebase exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all potential consumers of this map type
ast-grep --pattern 'MeasurementParticlesMap'
Length of output: 44
Script:
#!/bin/bash
# Search for files containing HitParticlesMap or MeasurementParticlesMap
rg -l "HitParticlesMap|MeasurementParticlesMap"
# Then examine the content of these files to see the usage
rg "HitParticlesMap|MeasurementParticlesMap" -B2 -A2
Length of output: 48535
Script:
#!/bin/bash
# Search for any ReadDataHandle with either type
ast-grep --pattern 'ReadDataHandle<$_ParticlesMap>'
Length of output: 53
Examples/Io/Root/include/ActsExamples/Io/Root/RootTrackParameterWriter.hpp (3)
12-12: Wise choice, the header change is, young padawan!
From Index.hpp to Measurement.hpp, the path leads. Align with the force of distinguishing measurements from hits, this change does.
34-35: Direct inheritance, clearer path it shows!
class RootTrackParameterWriter final : public WriterT<TrackParametersContainer>More explicit the inheritance has become. Type aliases, needed they are not. Wisdom in simplicity, there is.
88-91: Changed the data handles have been, hmmmm.
From hits to measurements, the transition flows. Reflect the true nature of the data, these changes do:
MeasurementParticlesMapinstead ofHitParticlesMapMeasurementSimHitsMapinstead ofHitSimHitsMap
With PR objectives, aligned perfectly these changes are. Strong with the Force, this refactoring is!
Verify the consistency across the codebase, we must:
✅ Verification successful
Consistent with the Force, these changes are
Searched the entire codebase, I have. Found many files using the new MeasurementParticlesMap and MeasurementSimHitsMap, I did. No lingering references to the old HitParticlesMap or HitSimHitsMap, there were. A complete and thorough transition, this is.
The path to enlightenment, this refactoring follows. Changed uniformly throughout the codebase, the naming has been. Strong with consistency, the code now is. Approve this change without hesitation, I do!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old hit maps
rg "HitParticlesMap|HitSimHitsMap" -l
# Ensure new measurement maps are used consistently
rg "MeasurementParticlesMap|MeasurementSimHitsMap" -l
Length of output: 3131
Script:
#!/bin/bash
# Search for specific old hit map references in content
rg "HitParticlesMap|HitSimHitsMap" -A 2 -B 2
# Look for any potential type definitions or aliases
ast-grep --pattern 'using $_ = $$$HitParticlesMap$$$'
ast-grep --pattern 'using $_ = $$$HitSimHitsMap$$$'
ast-grep --pattern 'typedef $$$HitParticlesMap$$$'
ast-grep --pattern 'typedef $$$HitSimHitsMap$$$'
Length of output: 260
Examples/Io/Root/src/RootTrackParameterWriter.cpp (1)
43-44: Approve base class change to WriterT template, hmmmm.
Wise choice it is, to use WriterT. Template-based approach, more type-safe it makes the writer implementation. Consistent with modern C++ practices, this change is.
Verify the impact of this change, we must:
✅ Verification successful
The previous pattern might have been too specific. Let us search with ripgrep to understand the context better, young padawan. Both the new template-based writer and the old writer class we shall examine, hmm.
Approve the change to WriterT template, I do. A pattern across the codebase, this is.
Searched through the Force, I have. Many writers using WriterT template base class, I found. A consistent pattern throughout the codebase, this change follows. RootTrackParameterWriter joins its fellow writers in using this template-based approach, hmmmm.
Examples of this pattern, I see:
- RootSimHitWriter using WriterT
- RootSeedWriter using WriterT
- RootVertexWriter using WriterT
- Many other writers following same template pattern
Wise decision this is, bringing consistency and type safety to the writer implementations. In harmony with the codebase's design philosophy, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other writers using the same pattern
ast-grep --pattern 'class $_ : public WriterT<$_> {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Search for files containing WriterT pattern with ripgrep for better context
rg "WriterT" -A 3 -B 3
# Also search for the old base class name to understand the migration
rg "TrackParameterWriter" -A 3 -B 3
Length of output: 72564
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (1)
Line range hint 75-167: Verify measurement handling across codebase, we must.
Ensure consistent handling of measurements throughout the codebase, we should. Search for other instances where hits and measurements might be confused, we must.
✅ Verification successful
Consistent handling of hits and measurements, I see.
Through the Force, searched the codebase I have. Clear distinction between hits and measurements, maintained it is:
- In
Particleclass,numberOfHitstracks physical interactions - In
TrackSelector, separatemeasurementCounterfor detector measurements exists - In
ParticleSelector, proper separation between hit count and measurement count, implemented it is - Throughout IO operations (Root readers/writers), consistent treatment of hits observed
No confusion or inconsistency in the handling of hits versus measurements, detected there is not. The changes in PR, properly aligned with the codebase conventions they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential confusion between hits and measurements in the codebase
echo "Searching for hit/measurement usage patterns..."
rg -A 2 "numberOfHits|measurementCount" --type cpp
echo "Checking for similar particle selectors..."
ast-grep --pattern 'class $_ : public $_ {
$$$
numberOfHits($$$) {
$$$
}
$$$
}'
Length of output: 8033
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.cpp (4)
28-36: Hmmmm, constructor changes, good they are.
Simplified the constructor signature has been, and proper error handling for particle-measurements map added it has. Clear and concise, the error messages are.
65-65: Initialize the particle measurements map, you must.
Consistent with the new naming convention, this initialization is. From hits to measurements, evolved our understanding has.
195-198: Clean and proper namespace handling, observed I have.
Consistent with the coding standards, the namespace closure is. Approve this change, I do.
116-123: Careful with measurements handling, we must be.
A critical section this is, where measurements for particles are gathered. But verify the performance impact of range creation, we should.
Consider this alternative implementation, more efficient it might be:
- const auto& measurements =
- makeRange(particleMeasurementsMap.equal_range(particle.particleId()));
- track.reserve(measurements.size());
- for (const auto& measurement : measurements) {
+ const auto [begin, end] = particleMeasurementsMap.equal_range(particle.particleId());
+ const size_t count = std::distance(begin, end);
+ track.reserve(count);
+ for (auto it = begin; it != end; ++it) {Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/SurfaceSortingAlgorithm.hpp (1)
49-51: Approve this change, I do. Yet verify its impact, we must.
Correct the type change from HitSimHitsMap to MeasurementSimHitsMap is, aligning with our mission to distinguish between hits and measurements it does.
Run this verification script, we shall:
✅ Verification successful
Complete, the migration to MeasurementSimHitsMap is. Approve this change with confidence, I do.
Searched the entire codebase, I have. No instances of the old HitSimHitsMap remain, while the new MeasurementSimHitsMap is consistently used throughout the codebase, hmm. In Examples/Framework/include/ActsExamples/EventData/Measurement.hpp, properly defined the type alias is:
using MeasurementSimHitsMap = IndexMultimap<Index>;Consistent usage across all components, I observe:
- Digitization algorithms
- Track fitting components
- IO operations (CSV, ROOT, EDM4hep)
- Python bindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of HitSimHitsMap to ensure complete migration
# Search for any remaining instances of old type
rg "HitSimHitsMap"
# Search for usage patterns of new type
rg "MeasurementSimHitsMap"
Length of output: 10526
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.hpp (2)
34-35: Clear and precise, this new terminology is.
From "measurement-particles" to "particle-measurements" we move. More accurate description of the relationship between particles and their measurements, this provides.
52-53: Strong with type safety, this change is.
The force of strong typing, we embrace. From generic HitParticlesMap to specific InverseMultimap<SimBarcode>, evolved we have.
Verify the usage patterns, we must:
✅ Verification successful
Strong in the Force, this change truly is!
Verified the type change has been, throughout the codebase. A consistent pattern we see:
- In Framework's Measurement.hpp, defined
ParticleMeasurementsMapasInverseMultimap<SimBarcode>is - In multiple truth-tracking algorithms, this type properly used it is:
- TruthTrackFinder
- TruthSeedingAlgorithm
- ParticleSelector
- DigitizationAlgorithm
No traces of the old HitParticlesMap remain, I sense. A complete and harmonious transition to InverseMultimap<SimBarcode>, achieved it has been. Balance to the type system, this brings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of InverseMultimap<SimBarcode>
# Search for all usages of InverseMultimap with SimBarcode
ast-grep --pattern 'InverseMultimap<SimBarcode>'
Length of output: 50
Script:
#!/bin/bash
# Hmmmm... Different approach, we shall take. Search for both old and new types, we must.
# First, the old type pattern seek
echo "=== Search for HitParticlesMap ==="
rg "HitParticlesMap" -A 2
# Then, the new InverseMultimap pattern investigate
echo -e "\n=== Search for InverseMultimap usage ==="
rg "InverseMultimap" -A 2
# The full file content examine, we shall
echo -e "\n=== The full context of TruthTrackFinder.hpp examine ==="
cat Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthTrackFinder.hpp
Length of output: 6690
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.hpp (2)
12-12: Wise choice of headers, this is.
From the path of SimHit to Measurement, our journey takes us. Aligned with our quest to separate hits from measurements, this change is.
57-58: In harmony with the force, this type change is.
From HitParticlesMap to MeasurementParticlesMap, transformed we have. Consistency across the codebase, this brings.
Verify the completeness of our transformation, we shall:
✅ Verification successful
Complete, our transformation is. Successful, the change from HitParticlesMap to MeasurementParticlesMap was.
Searched through the entire codebase, I have. No remaining instances of HitParticlesMap found, they were. The new type MeasurementParticlesMap consistently used across the codebase, it is. Though some variables named hitParticlesMap remain in implementation files, their type declarations correct they are, using MeasurementParticlesMap.
In balance with the Force, this change is. Proceed with confidence, we may.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure consistent type usage across the codebase
# Search for any remaining instances of HitParticlesMap
rg "HitParticlesMap"
# Verify usage of new MeasurementParticlesMap
rg "MeasurementParticlesMap"
Length of output: 11912
Examples/Framework/include/ActsExamples/EventData/Index.hpp (2)
33-40: Approve the new type alias, I do! Clear and purposeful, it is.
Wise decision to introduce InverseMultimap type alias, making the code more maintainable and self-documenting, it does. Well-documented with clear purpose, the changes are.
42-55: Function signature improvement, I sense. Good changes, these are.
Updated to use new type alias, the function remains functionally equivalent. Clearer intent and better maintainability, it provides. The Force, strong with this code, it is.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.hpp (2)
33-34: Renamed variable, more accurate it becomes.
From inputMeasurementParticlesMap to inputParticleMeasurementsMap, the change flows. Better describes the relationship between particles and their measurements, it does.
73-74: Type update, wisdom it shows.
From HitParticlesMap to InverseMultimap<SimBarcode>, the transition makes. Consistent with the new type system and clearer in purpose, it becomes. The Force guides this change well.
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSeedWriter.hpp (2)
11-11: Include Measurement.hpp, necessary it is.
Added dependency on measurement functionality, explicit it becomes. Clear intentions, this shows.
80-83: Types updated, clarity they bring.
From hit-centric to measurement-centric types, the transition flows. Consistent with the broader changes across the codebase, these updates are. Handle measurements with proper types, we now do.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp (2)
58-60: Separate hit counts from measurements, you have. Good, this is!
Clear distinction between hits and measurements, maintain we now do. Proper initialization with sensible defaults, you have provided.
30-32: Wise addition of particle measurements map, this is!
Clear documentation and optional nature, maintain flexibility it does.
Verify the usage of this new input, we must:
✅ Verification successful
Properly implemented, the new input map is!
Verified the implementation has been, hmm. Wisely handled, it is:
- Optional initialization through
maybeInitialize, the map receives - Gracefully falls back to empty map when not initialized, it does
- Proper validation enforces measurement-based cuts with required map, it does
The Force is strong with this implementation, clear error handling it has, and flexible usage it provides! No disturbance in the codebase, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for configurations of ParticleSelector to ensure proper usage
rg -l "ParticleSelector.*Config" | xargs rg "inputParticleMeasurementsMap"
Length of output: 1022
Script:
#!/bin/bash
# Search for the implementation to understand how the map is used
ast-grep --pattern 'if (m_inputParticleMeasurementsMap.isInitialized() $_) {
$$$
}'
Length of output: 87
Script:
#!/bin/bash
# Check if there are any configuration examples
rg -A 5 "ParticleSelector::Config.*inputParticleMeasurementsMap"
Length of output: 66
Script:
#!/bin/bash
# Look for test cases
fd ".*test.*" --exec rg "inputParticleMeasurementsMap" {}
Length of output: 59
Script:
#!/bin/bash
# Search for the implementation with more context
rg -B 5 -A 10 "m_inputParticleMeasurementsMap" Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp
Length of output: 1621
Script:
#!/bin/bash
# Look for any configuration setup in Python scripts
fd ".py$" --exec rg "inputParticleMeasurementsMap" {}
Length of output: 55
Examples/Python/src/Digitization.cpp (1)
58-59: Python bindings, properly extended they are!
Expose the new mappings to Python users, this does. Consistent with C++ interface, it remains.
Examples/Algorithms/Digitization/include/ActsExamples/Digitization/DigitizationAlgorithm.hpp (2)
51-54: Output mappings, well-named they are!
Clear distinction between particle-to-measurement and simhit-to-measurement mappings, maintain you do. Default values, sensible they are.
147-151: Handle the output mappings correctly, you do!
Proper use of InverseMultimap, demonstrated you have. Consistent with ACTS framework patterns, this remains.
Verify the usage of these handles in implementation, we must:
✅ Verification successful
Properly implemented, the handle usage is!
In the implementation file, see I do:
- Initialized correctly in the setup phase, the handles are
- Called appropriately with context and inverted maps, they become
- The Force flows strong through this code, hmm, yes!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the handle usage in implementation
rg -A 5 "OutputParticleMeasurementsMap|OutputSimHitMeasurementsMap" Examples/Algorithms/Digitization/src/
Length of output: 107
Script:
#!/bin/bash
# Search for implementation files first
fd "DigitizationAlgorithm" Examples/Algorithms/Digitization/src/
# Then search for handle usage with proper path
rg -A 5 "OutputParticleMeasurementsMap|OutputSimHitMeasurementsMap" Examples/Algorithms/Digitization/
Length of output: 1284
Script:
#!/bin/bash
# Search for handle usage in the implementation file
rg -A 10 "m_outputParticleMeasurementsMap|m_outputSimHitMeasurementsMap" Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp
Length of output: 849
Examples/Python/src/TruthTracking.cpp (2)
43-43: Approve parameter renaming for clarity, hmm yes.
Clearer now the relationship between particles and measurements is, with the new parameter names. Consistent with the codebase's direction, this change is.
Also applies to: 164-164
87-88: Wise addition of hit count parameters, this is.
For filtering particles based on hit counts, these parameters are. Separate from measurement counts they now stand, as the ancient scrolls of PR #3742 intended.
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp (2)
61-68: Strong with validation, this code has become.
Proper checks for output collections, added they are. Missing configurations, caught they will be.
152-152: Bidirectional mapping, achieved it has.
Through the force of inverted maps, both directions of relationships we now see. From measurements to particles and back again, flow the data will.
Yet verify the inversion logic, we must:
Also applies to: 315-319
✅ Verification successful
Correct implementation of bidirectional mapping, I sense.
Examined the Force flows have I, and proper balance they maintain. The invertIndexMultimap function, consistent in its ways across the codebase it is. From measurements to particles and back again, the mapping flows as intended it does. Through boost::container::flat_multimap, efficient the implementation remains.
In all places where used it is:
DigitizationAlgorithm.cpp: For particle measurements and simulated hits, properly inverted they areRootAthenaDumpReader.cpp: For measurement-particle relationships, correctly transformed they become
Trust in the Force of this implementation, we can. Hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of invertIndexMultimap to ensure consistent usage
rg "invertIndexMultimap" --type cpp
Length of output: 520
Script:
#!/bin/bash
# Check the implementation of invertIndexMultimap
ast-grep --pattern 'inline InverseMultimap<$_> invertIndexMultimap($_) { $$$ }'
# Check usage context in DigitizationAlgorithm
rg -B 5 -A 5 "invertIndexMultimap\(measurementParticlesMap\)" Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp
# Check usage context in RootAthenaDumpReader
rg -B 5 -A 5 "invertIndexMultimap\(measPartMap\)" Examples/Io/Root/src/RootAthenaDumpReader.cpp
Length of output: 1103
Script:
#!/bin/bash
# Check the implementation details of IndexMultimap and InverseMultimap
rg -B 2 -A 10 "class (IndexMultimap|InverseMultimap)" Examples/Framework/include/ActsExamples/EventData/Index.hpp
Length of output: 115
Script:
#!/bin/bash
# Try different pattern to find type definitions
rg -B 2 -A 10 "using (IndexMultimap|InverseMultimap)" Examples/Framework/include/ActsExamples/EventData/Index.hpp
# Also check for template definitions
ast-grep --pattern 'template<$_> using IndexMultimap = $_'
ast-grep --pattern 'template<$_> using InverseMultimap = $_'
Length of output: 1220
Examples/Python/python/acts/examples/simulation.py (2)
397-401: Correctly implemented, the new configuration parameters are!
Proper handling of both hits and measurements ranges, I see. Good separation of concerns, this maintains.
44-45: Approve the distinction between hits and measurements, I do!
Wise changes these are, separating 'hits' from 'measurements' in the configuration. Clear distinction now exists, preventing confusion there will be.
Verify the usage of these new fields across the codebase, we must:
✅ Verification successful
Verified the usage of hits and measurements fields, I have!
Found evidence of proper usage throughout the codebase, I did. The new fields are correctly utilized in various scripts, particularly in postSelectParticles configurations. Examples of wisdom, I share:
- In
Examples/Scripts/Python/truth_tracking_kalman.py, measurements field is used:measurements=(7, None) - In
Examples/Scripts/Python/ckf_tracks.py, measurements field appears:measurements=(9, None) - No conflicts or incorrect usage detected, there are
The Force is strong with these changes. Clear separation between hits and measurements, maintained it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of hits and measurements in particle selection
rg -A 2 "ParticleSelectorConfig"
Length of output: 16691
github-actions
bot
added
Component - Examples
andiwand
force-pushed
the
ex-particle-selector-hits-vs-measurements
branch
from
3ca47bd to
dec7c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp (1)
Line range hint
1-96: Architecture advice, share with you I must!Consider these suggestions, young padawan:
- Document behavior when
inputParticleMeasurementsMapis absent, you should- Add static_assert for relationship between hits and measurements limits, wise it would be
- Consider adding validation method for configuration, helpful it would be
Strong in the Force, this refactoring is. But documentation for edge cases, strengthen it we must.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (3)
33-42: Approve the initialization and validation logic, I do. But strengthen it further, we must.Correct the initialization and validation are, ensuring measurement-based cuts work only when the map exists. However, suggest additional validation for the measurement thresholds, I do.
Add this validation, you should:
if (m_inputParticleMeasurementsMap.isInitialized() && (m_cfg.measurementsMin > 0 || m_cfg.measurementsMax < std::numeric_limits<std::size_t>::max())) { throw std::invalid_argument( "Measurement-based cuts require the inputMeasurementParticlesMap"); } +if (m_cfg.measurementsMin > m_cfg.measurementsMax) { + throw std::invalid_argument( + "Minimum measurements cannot exceed maximum measurements"); +} +if (m_cfg.hitsMin > m_cfg.hitsMax) { + throw std::invalid_argument( + "Minimum hits cannot exceed maximum hits"); +}
79-84: Clever the static empty map solution is, but document it we should.A clean fallback mechanism this provides, but its purpose should be documented for future maintainers.
Add this comment, you should:
+// Static empty map used as fallback when measurement map is not initialized const static InverseMultimap<SimBarcode> emptyMeasurementParticlesMap;
Line range hint
75-167: Wise architectural decision, this separation of hits and measurements is.Align with the single responsibility principle, these changes do. Each metric (hits and measurements) now serves its distinct purpose. However, consider these architectural points:
- Document the difference between hits and measurements in the class documentation
- Consider creating separate configuration structs for hit-based and measurement-based filtering
Help with the documentation or configuration refactoring, would you like?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp(3 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp(7 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp(4 hunks)Examples/Python/python/acts/examples/reconstruction.py(3 hunks)Examples/Python/python/acts/examples/simulation.py(3 hunks)Examples/Python/src/Output.cpp(1 hunks)Examples/Python/src/TruthTracking.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/Python/src/TruthTracking.cpp
- Examples/Python/src/Output.cpp
🔇 Additional comments (12)
Examples/Python/python/acts/examples/reconstruction.py (2)
583-583: Approve the parameter renaming in addTruthSmearedSeeding, I do.
Clearer naming convention this is, hmm. Better distinction between particles and their measurements, it provides.
604-604: Approve the parameter renaming in addTruthEstimatedSeeding, I do.
Consistent with the changes above, this renaming is. Maintain clarity throughout the codebase, it does.
Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp (3)
61-68: Hmmmm, proper validation checks these are!
Wise addition of error checks for mapping configurations, prevent silent failures it does.
75-77: Strong with the Force, this initialization is!
Properly initialized, the new output handles are. Balance to the code, they bring.
313-318: Hmmmm, proper handling of mappings, I sense!
Inverted and written correctly, the measurement maps are. But verify their usage across the codebase, we must.
✅ Verification successful
Correct and complete, the mapping implementation is!
Clear it is, that these mappings serve their purpose well. Initialize they do in constructor, and write they do only in one place, with proper inversion before moving. The Force is strong with this implementation, as:
InverseMultimaptype they use, matching their purpose- Properly initialized in constructor, they are
- Written once only, during clusterization phase
- Inverted correctly before storage, maintaining proper relationships they are
Through the Force, I sense no disturbance in these mappings. Safe to proceed, we are!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these mappings in the codebase
rg "particle_measurements_map|simhit_measurements_map" --type cpp
Length of output: 403
Script:
#!/bin/bash
# Search for usage of m_outputParticleMeasurementsMap and m_outputSimHitMeasurementsMap
ast-grep --pattern 'm_outputParticleMeasurementsMap'
ast-grep --pattern 'm_outputSimHitMeasurementsMap'
# Check for any write context to these maps
rg -B 5 -A 5 "m_outputParticleMeasurementsMap|m_outputSimHitMeasurementsMap" --type cpp
Length of output: 5298
Examples/Python/python/acts/examples/simulation.py (2)
44-44: Wise addition to the configuration, this is!
Separate tracking of hits and measurements, crucial it is. Clear distinction between them, this brings.
817-818: Balanced and harmonious, these new mappings are!
Complete, the distinction between hits and measurements becomes. Properly configured, the output paths are.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp (3)
12-12: Approve the include addition, I do!
Necessary for Index.hpp inclusion, this is. Proper dependency management, you have shown.
30-32: Clear documentation for optional input, you have provided!
Well documented the optional nature of particle measurements map, you have. Clear purpose in comments, I see. Good practices in documentation, you follow.
58-60: Distinguish between hits and measurements, we must!
Hmm, separate concepts these are - hits and measurements. Wise decision to track them separately, you have made. Default values sensible they are.
Yet verify the usage across the codebase, we should.
✅ Verification successful
Let me analyze the codebase further to verify the distinction between hits and measurements.
Wise distinction between hits and measurements, confirmed it is!
Through the Force, clear understanding I have gained:
- Hits ("SimHit", "Hit") represent truth-level interactions where particles cross detector surfaces, before any digitization or reconstruction. Raw physical interactions they are, hmm.
- Measurements represent reconstructed detector responses after digitization and clustering. More processed and closer to real detector output, they are.
In the ParticleSelector.hpp, separate counters for both concepts exist:
// Hit count cuts
std::size_t hitsMin = 0;
std::size_t hitsMax = std::numeric_limits<std::size_t>::max();
// Measurement number cuts
std::size_t measurementsMin = 0;
std::size_t measurementsMax = std::numeric_limits<std::size_t>::max();Correct the original review comment was - sensible defaults these values have. Track both quantities separately we must, as different purposes they serve in the tracking chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of hits vs measurements terminology
# Check for potential confusion between hits and measurements
rg -A 3 "(hits|measurements)" --type cpp
Length of output: 67197
Script:
#!/bin/bash
# Search for definitions and usage patterns of hits vs measurements
# Look for class/struct definitions and documentation
ast-grep --pattern 'class $_ {
$$$
hits$_
$$$
}'
ast-grep --pattern 'struct $_ {
$$$
hits$_
$$$
}'
# Search for documentation and comments about hits vs measurements
rg -B2 -A2 "(hit|measurement)" --type cpp --glob "**/*.h" --glob "**/*.hpp"
Length of output: 79716
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (2)
60-61: Approve the debug logging addition, I do.
Clear and consistent with other parameter logging, this addition is. Help in debugging particle selection criteria, it will.
109-116: Clear separation between hits and measurements, this achieves.
Correctly distinguish between particle hits and associated measurements, this implementation does. But verify the impact on performance, we must.
Run this script to analyze the performance impact:
✅ Verification successful
Performance impact, minimal it is
Through the Force, seen I have that count() operation on inputMeasurementParticlesMap performs efficiently. A simple lookup in the map it is, used widely across the codebase for truth matching and validation purposes. No performance bottlenecks in the implementation, there are.
- Used consistently across multiple components for truth tracking and validation, the pattern is
- O(1) complexity for map lookups, maintained it has
- No recursive operations or expensive computations in the measurement counting logic, found I have
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance bottlenecks in measurement counting
# Search for other uses of measurement counting patterns
rg -A 3 "count\(.*particleId\(\)"
# Look for alternative implementations
ast-grep --pattern 'count($x.particleId())'
Length of output: 620
Script:
#!/bin/bash
# Search for the implementation of the map to understand its type and usage
rg -B 5 -A 2 "inputMeasurementParticlesMap"
# Look for performance-critical sections or benchmarks
fd "Benchmark|Performance" --type f --exec rg -l "ParticleSelector|measurement.*count"
# Check if there are any comments or documentation about performance considerations
rg -B 2 -A 2 "Performance|TODO|FIXME|NOTE" Examples/Algorithms/TruthTracking/
Length of output: 52142
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp
Show resolved
Hide resolved
andiwand
force-pushed
the
ex-particle-selector-hits-vs-measurements
branch
from
dec7c97 to
df40a9d
Compare
github-actions
bot
added
Infrastructure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
Examples/Scripts/Python/seeding.py (2)
93-94: Document the wisdom of minimum hits, we should!Consistent with other files, the minimum of 9 hits is. Yet, document why this magic number we chose, we must. A comment explaining the significance of 9 hits, helpful it would be.
postSelectParticles=ParticleSelectorConfig( pt=(1.0 * u.GeV, None), eta=(-2.5, 2.5), - hits=(9, None), + # Minimum 9 hits required for reliable track reconstruction + hits=(9, None), removeNeutral=True, ),
Line range hint
72-73: Centralize the configuration, we must!Repeated across multiple files, the magic number 9 for minimum hits is. Consider creating a shared configuration module, we should. Constants for these values, define there we must. Make maintenance easier and consistency stronger, this will.
Help create this shared configuration module, shall I? Open a GitHub issue for this task, I can.
Also applies to: 77-78, 93-94
CI/physmon/workflows/physmon_trackfinding_1muon.py (1)
77-77: Different minimum values across files, noticed I have.In GSF and GX2F examples, 7 hits we require. Yet here, 9 hits we demand. Document these differences in requirements, we should.
Add a comment explaining the physics motivation:
postSelectParticles=ParticleSelectorConfig( pt=(0.9 * u.GeV, None), + # Require minimum 9 hits for 1-muon physics monitoring to ensure high-quality tracks hits=(9, None), removeNeutral=True, ),CI/physmon/workflows/physmon_trackfinding_4muon_50vertices.py (1)
74-74: Wisdom of the architectural ways, share I must!Different minimum hits values across tracking methods, observe we do:
- CKF tracking: 9 hits minimum
- Truth tracking: 7 hits minimum
- Physics monitoring: 9 hits minimum
A pattern this reveals - stricter requirements for production tracking than truth tracking. Wise this is, as truth tracking, perfect knowledge it has.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (1)
79-83: Static empty map, wisely placed it is!Avoid unnecessary allocations when measurement map not needed, this implementation does. Yet, improve it further, we can.
Consider marking the empty map as
const:-const static InverseMultimap<SimBarcode> emptyMeasurementParticlesMap; +static const InverseMultimap<SimBarcode> emptyMeasurementParticlesMap;Examples/Scripts/Python/full_chain_odd.py (1)
281-281: Wise changes these are, hmm yes.The path to clarity, this change brings. Between hits and measurements, distinction now clear it is. In both Geant4 and Fatras paths, balanced the force remains.
Consider documenting the distinction between hits and measurements in the configuration files, help future Padawans it will.
Also applies to: 309-309
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
CI/physmon/workflows/physmon_trackfinding_1muon.py(1 hunks)CI/physmon/workflows/physmon_trackfinding_4muon_50vertices.py(1 hunks)CI/physmon/workflows/physmon_trackfinding_ttbar_pu200.py(1 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp(7 hunks)Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp(4 hunks)Examples/Python/python/acts/examples/simulation.py(2 hunks)Examples/Python/src/TruthTracking.cpp(1 hunks)Examples/Python/tests/test_examples.py(1 hunks)Examples/Scripts/Optimization/ckf.py(1 hunks)Examples/Scripts/Python/ckf_tracks.py(1 hunks)Examples/Scripts/Python/full_chain_itk.py(1 hunks)Examples/Scripts/Python/full_chain_itk_Gbts.py(1 hunks)Examples/Scripts/Python/full_chain_odd.py(2 hunks)Examples/Scripts/Python/full_chain_odd_LRT.py(2 hunks)Examples/Scripts/Python/full_chain_test.py(1 hunks)Examples/Scripts/Python/hashing_seeding.py(1 hunks)Examples/Scripts/Python/seeding.py(1 hunks)Examples/Scripts/Python/truth_tracking_gsf.py(1 hunks)Examples/Scripts/Python/truth_tracking_gx2f.py(1 hunks)Examples/Scripts/Python/truth_tracking_kalman.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/Python/src/TruthTracking.cpp
- Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.hpp
👮 Files not reviewed due to content moderation or server errors (3)
- Examples/Scripts/Python/full_chain_test.py
- Examples/Python/python/acts/examples/simulation.py
- Examples/Python/tests/test_examples.py
🔇 Additional comments (15)
Examples/Scripts/Python/full_chain_itk_Gbts.py (1)
72-73: Hmmmm, consistent with CKF configuration, verify we must!
Notice I do, that minimum hits of 9 you set, yet in CKF tracks configuration, nMeasurementsMin of 6 exists. Wisdom suggests, these values we should align.
✅ Verification successful
Hmmmm, consistent these configurations are, young padawan!
Through the Force, I sense harmony between the configurations:
- In
full_chain_itk_Gbts.py, minimum hits of 9 for particle selection, a wise choice it is - For CKF configuration in same file, nMeasurementsMin=6, correctly set it remains
- Natural this difference is, as hits count includes both measurements and holes, while nMeasurementsMin counts only measurements
Throughout the codebase, this pattern repeats:
- Most CKF configurations use nMeasurementsMin of 6 or 7
- Track selection configs often require more hits (8-9) for quality assurance
Balance to the Force, these values bring. Approve this change, I do!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all nMeasurementsMin configurations in the codebase
rg "nMeasurementsMin\s*=\s*\d+" -A 2
Length of output: 8192
Examples/Scripts/Python/full_chain_itk.py (1)
77-78: Complex track selection criteria, examine we must!
Set minimum hits to 9 you have, yet multiple track selection configurations exist below with varying nMeasurementsMin (9, 8, and 7). Consider aligning these values with track selection tiers, we should.
✅ Verification successful
Aligned with the Force, these track selections are!
In full_chain_itk.py, observe a deliberate tier system we do:
- Tier 1 (|η| ≤ 2.0): 9 hits, pt > 0.9 GeV
- Tier 2 (|η| ≤ 2.6): 8 hits, pt > 0.4 GeV
- Tier 3 (|η| ≤ 4.0): 7 hits, pt > 0.4 GeV
Wise design this is! As tracks venture into higher |η| regions, fewer hits expected they are, hence lower requirements we set. The minimum hits of 9 in particle selector harmoniously aligns with the strictest tier. Balance in the Force, I sense!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for track selection configurations in the file
ast-grep --pattern 'TrackSelectorConfig($$$)'
Length of output: 6355
Examples/Scripts/Python/truth_tracking_gsf.py (1)
82-82: Hmmmm, inconsistent terminology between selectors, I sense.
Between ParticleSelectorConfig using 'hits' and TrackSelectorAlgorithm using 'minMeasurements', confusion there may be. Verify the distinction between hits and measurements in both selectors, we must.
CI/physmon/workflows/physmon_trackfinding_1muon.py (1)
77-77: Inconsistent validation criteria between particle and track selection, troubling this is.
ParticleSelectorConfig requires 9 hits, while TrackSelectorConfig requires only 6 measurements. Clarify this difference or align these values, we must.
Also applies to: 144-148
✅ Verification successful
Let me search for these configurations to verify the discrepancy between particle and track selection criteria, hmm.
Different requirements serve different purposes, they do. Valid, this configuration is.
Understand now, I do. The particle selector with 9 hits ensures quality of simulated particles, while track selector's 6 measurements requirement applies to reconstructed tracks - different stages of the Force, they represent. Natural this difference is, for:
- Particle selection happens at simulation level, requiring 9 hits for clean particle trajectories
- Track selection occurs during reconstruction, where 6 measurements sufficient are, accounting for detector inefficiencies and physical constraints
The ways of the Force balanced they are, each requirement serving its purpose in the cosmic dance of particle physics. No changes needed, hmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all hit/measurement requirements across configs
rg "(?:hits|measurements).*=.*\d+" --type py
Length of output: 1883
Script:
#!/bin/bash
# Search for TrackSelectorConfig and ParticleSelectorConfig definitions and their usage
ast-grep --pattern 'class $_(ParticleSelectorConfig|TrackSelectorConfig) {
$$$
}'
# Also search for their configuration in the specific workflow
rg -A 5 "(?:ParticleSelectorConfig|TrackSelectorConfig)" CI/physmon/workflows/physmon_trackfinding_1muon.py
Length of output: 808
Examples/Scripts/Python/ckf_tracks.py (1)
85-85: Approve this change, I do! Yet verify the minimum hits we must.
Correct the parameter change from measurements to hits is, hmm. A minimum of 9 hits, reasonable it seems, but verify this threshold we should.
✅ Verification successful
Verified the minimum hits threshold is, hmm! Consistent across the codebase it remains.
Through the Force, I sense that 9 hits is indeed the standard configuration for most tracking scenarios, yes. Only in specialized truth tracking cases (GSF, Kalman, GX2F) do we see a lower threshold of 7 hits. For this CKF (Combinatorial Kalman Filter) tracking configuration, the 9-hit requirement aligns perfectly with other similar tracking configurations and production workflows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other minimum hit configurations in the codebase
rg -A 1 "hits=\([0-9]+"
Length of output: 2804
Examples/Scripts/Python/truth_tracking_kalman.py (1)
87-87: Consistent with the Force, this change is!
From measurements to hits, the parameter name changes. Different from CKF tracking, a minimum of 7 hits this truth tracking requires. Acceptable this difference is, as different paths to the Force, these tracking methods follow.
CI/physmon/workflows/physmon_trackfinding_4muon_50vertices.py (1)
74-74: In harmony with the Force, this change flows!
The path from measurements to hits, consistent it is. With CKF tracking, the minimum of 9 hits aligns. Balance to the codebase, this brings.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (3)
60-61: Enhanced logging, you have provided!
Clear visibility into hit count ranges, this debug message gives. Help in troubleshooting, it will.
109-116: Clear separation between hits and measurements, achieved you have!
Distinct validation for hits and measurements, properly implemented it is. Clear and maintainable, the code remains.
33-42: Wise initialization and validation, this is!
Proper checks for measurement map initialization before applying measurement-based cuts, you have added. Prevent runtime errors, this will.
Verify other measurement-based validations in the codebase, we must:
✅ Verification successful
Let me search for validation patterns with different approaches, as the Force guides us to explore multiple paths. The previous pattern might have been too specific, so we shall cast a wider net to find similar safety checks in the codebase.
Hmm, the Force tells me we must look deeper into the TruthTracking files, particularly around the ParticleSelector implementation. The previous search patterns were too broad, now we shall focus our search on the specific files that matter.
Wise validation check, this is indeed!
Examined the codebase thoroughly, I have. The validation in ParticleSelector.cpp, correct and necessary it is. When measurement-based cuts are requested but measurement map not provided, throw an error it must. No other similar validations in the codebase exist, as unique to this use case, this check is.
The Force shows me that:
- The check prevents invalid configuration where measurement cuts would be meaningless without measurement data
- Properly initialized before use, the measurement map must be
- No other components require similar validation, as this specific to ParticleSelector functionality is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar validation patterns
ast-grep --pattern 'if ($map.isInitialized() && ($condition)) {
throw std::invalid_argument($_);
}'
Length of output: 103
Script:
#!/bin/bash
# Search for measurement-based validations with different patterns
rg -U "measurementsM(in|ax).+(throw|invalid_argument)" -A 2
# Search for isInitialized checks
rg -U "isInitialized.+(throw|invalid_argument)" -A 2
# Look for similar validation patterns in the TruthTracking directory
fd "\.cpp$" -e cpp --exec grep -l "invalid_argument" {} | xargs cat
Length of output: 72101
Script:
#!/bin/bash
# Search for measurement-based validations in TruthTracking directory
fd "TruthTracking" -t f -x grep -l "measurementsM" {} | xargs cat
# Search for isInitialized checks in TruthTracking directory
fd "TruthTracking" -t f -x grep -l "isInitialized" {} | xargs cat
# Look specifically in ParticleSelector files
fd "ParticleSelector" -t f | xargs cat
Length of output: 23525
CI/physmon/workflows/physmon_trackfinding_ttbar_pu200.py (1)
74-74: Aligned with core changes, this configuration is!
From measurements to hits, the parameter name changed has been. Consistency across codebase, this maintains.
Examples/Scripts/Optimization/ckf.py (1)
174-174: In harmony with core changes, this configuration stands!
From measurements to hits, the parameter name changed has been. Consistency across optimization scripts, this maintains.
Examples/Scripts/Python/hashing_seeding.py (1)
218-218: Correct, the parameter change is, hmm yes.
From measurements to hits, the change aligns with the force of consistency, it does. A minimum of 9 hits, a wise choice this is.
✅ Verification successful
Consistent in the Force, this change is, hmmmm.
Through the codebase, searched I have. The parameter hits=(9, None) appears uniformly across many files, it does. No traces of the old measurements parameter remain, and the new terminology aligns with the greater purpose, yes.
A pattern of wisdom I see:
- In Examples/Scripts/Python/hashing_seeding.py, the change follows the way of other files
- In CI workflows, test files, and example scripts, the same configuration of
hits=(9, None)exists - The old parameter
measurements, found nowhere it is
The Force is strong with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of hits parameter across codebase
rg -g '*.py' 'ParticleSelectorConfig.*hits.*=.*\(9,'
Length of output: 54
Script:
#!/bin/bash
# Search for hits parameter in Python files with more context
rg -g '*.py' -B 2 -A 2 'hits\s*=\s*\(9,'
# Find ParticleSelector configuration patterns
ast-grep --pattern 'ParticleSelectorConfig($$$)'
# Look for measurements parameter to verify the change
rg -g '*.py' -B 2 -A 2 'measurements\s*=\s*\(9,'
Length of output: 20328
Examples/Scripts/Python/full_chain_odd_LRT.py (1)
276-276: Balanced and harmonious, these changes are.
In both Geant4 and Fatras configurations, consistent the implementation is. The force of proper hit counting, strong it flows.
Also applies to: 303-303
Examples/Scripts/Python/full_chain_odd.py (1)
281-281: Verify the complete transition, we must.
Search for any remaining instances of the old ways, important it is.
Also applies to: 309-309
✅ Verification successful
The Force guides me to broaden our search patterns, as the specific class name might vary. Let us examine the codebase with a wider lens, we shall.
Consistent in the Force, these changes are.
Examined the codebase thoroughly, I have. Two patterns emerge, they do:
- Most files use
hits=(9, None)for standard tracking - Truth tracking files use
hits=(7, None)for specific cases
A natural balance in the Force this represents, hmm. Different requirements for different tracking methods, expected this is. No inconsistencies to fix, there are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to measurements in particle selection
rg -g '*.py' 'ParticleSelectorConfig.*measurements.*=.*\('
# Verify consistent minimum hits requirement
rg -g '*.py' 'ParticleSelectorConfig.*hits.*=.*\('
Length of output: 113
Script:
#!/bin/bash
# Search for any remaining references to measurements in particle selection with broader pattern
rg -g '*.py' 'measurements.*=.*\(' -A 2 -B 2
# Search for hits configuration patterns more broadly
rg -g '*.py' 'hits.*=.*\(' -A 2 -B 2
# Find all Python files that might contain particle selection configs
fd '.*\.py$' --exec grep -l "ParticleSelector" {} \;
Length of output: 8679
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (2)
109-116: Strengthen the validation of hit and measurement counts, we should!Good separation between hits and measurements, I sense. But stronger validation, we need.
Consider this improvement:
+ // Ensure hit count is not less than measurement count + if (p.numberOfHits() < measurementCount) { + ACTS_WARNING("Inconsistent state: hits < measurements for particle " + << p.particleId()); + } const bool validHitCount = within(p.numberOfHits(), m_cfg.hitsMin, m_cfg.hitsMax); nInvalidHitCount += static_cast<std::size_t>(!validHitCount);
159-161: Enhance debug logging with more details, we shall!Good logging you have, but more informative it could be.
Apply this enhancement:
- ACTS_DEBUG("filtered out because of hit count: " << nInvalidHitCount); - ACTS_DEBUG("filtered out because of measurement count: " - << nInvalidMeasurementCount); + ACTS_DEBUG("filtered out because of hit count (" << m_cfg.hitsMin + << " <= hits < " << m_cfg.hitsMax << "): " << nInvalidHitCount); + ACTS_DEBUG("filtered out because of measurement count (" + << m_cfg.measurementsMin << " <= measurements < " + << m_cfg.measurementsMax << "): " << nInvalidMeasurementCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp(7 hunks)
🔇 Additional comments (2)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSelector.cpp (2)
37-42:
Fix the condition in measurement map validation, you must!
A negation in the condition missing there is, young padawan. Previous review comment this issue spotted has.
Apply this fix, you should:
- if (!m_inputParticleMeasurementsMap.isInitialized() &&
+ if (!m_inputParticleMeasurementsMap.isInitialized() &&
(m_cfg.measurementsMin > 0 ||
m_cfg.measurementsMax < std::numeric_limits<std::size_t>::max())) {79-83: Thread safety of static variable, consider you must!
Static empty map as fallback, wise choice it is. But in multi-threaded environment, initialization race condition possible it is.
Run this command to check for concurrent usage:
Consider making the empty map const static inline or using Meyer's Singleton pattern, if thread safety needed it is.
|
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:
DigitizationAlgorithmin Examples #3944Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores