feat!: Updating the cuts in ScoreBased Solver to eta based cuts. #4054
Conversation
removed unused varible
|
Warning Rate limit exceeded@Ragansu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
## Walkthrough
Significant modifications made to the `ScoreBasedAmbiguityResolution` class across multiple files, they are. The focus on enhancing configuration and scoring mechanisms for track resolution, it is. Key transformations include removal of specific momentum and angular parameters, introduction of a new `minUnshared` parameter, and implementation of more granular eta-based configuration options for track scoring, they do.
## Changes
| File | Change Summary |
|------|----------------|
| `Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp` | Restructured `DetectorConfig` and `Config` structs; added eta-based configuration vectors. |
| `Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp` | Simplified scoring methods; removed pT, phi, eta checks. |
| `Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp` | Added method `etaBasedCuts` for eta-based scoring. |
| `Examples/Algorithms/AmbiguityResolution/...` | Removed angular and momentum parameters; added `minUnshared`. |
| `Examples/Python/...` | Updated configuration for ambiguity resolution; simplified parameter set. |
| `Examples/Scripts/Python/...` | Modified ambiguity resolution configuration across multiple scripts. |
| `Plugins/Json/src/AmbiguityConfigJsonConverter.cpp` | Updated `from_json` function to reflect new detector configuration parameters. |
| `Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp` | Adjusted test configuration by adding `minUnshared` and removing old parameters. |
## Sequence Diagram
```mermaid
sequenceDiagram
participant Tracks
participant AmbiguityResolver
participant DetectorConfig
Tracks->>AmbiguityResolver: Submit tracks
AmbiguityResolver->>DetectorConfig: Request eta-based cuts
DetectorConfig-->>AmbiguityResolver: Return cut parameters
AmbiguityResolver->>AmbiguityResolver: Calculate track scores
AmbiguityResolver->>Tracks: Return resolved tracksPossibly related PRs
Suggested Labels
Suggested Reviewers
Poem
|
github-actions
bot
added
Component - Core
Ragansu
changed the title
Ragansu
changed the title
![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
🧹 Nitpick comments (8)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (2)
116-117: New parameter for minimum unshared hits, added it is.
Clear documentation highlight, you should. Value of 5 might not suit all detectors, test you must.
227-230: Helper function to retrieve cuts per eta bin, neat it is.
Yet defensive checks for invalid bin index, implement you might.Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp (1)
234-238: Abundant debug logs, user-friendly they are.
In production, overhead consider you must.Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp (1)
30-59: Optimize etaBasedCuts implementation, we should.Improvements needed, I sense:
- Early return when no cuts applied, there should be
- Binary search for eta bin finding, more efficient it would be
- Redundant boolean operations, eliminated they must be
bool Acts::ScoreBasedAmbiguityResolution::etaBasedCuts( const DetectorConfig& detector, const TrackFeatures& trackFeatures, const double& eta) const { std::vector<double> etaBins = detector.etaBins; - int etaBin = 0; - bool cutApplied = false; + auto it = std::upper_bound(etaBins.begin(), etaBins.end(), eta); + if (it == etaBins.begin() || it == etaBins.end()) { + return false; // eta out of range + } + size_t etaBin = std::distance(etaBins.begin(), it) - 1; - for (std::size_t i = 0; i < etaBins.size() - 1; ++i) { - if (eta >= etaBins[i] && eta < etaBins[i + 1]) { - etaBin = i; - break; - } - } - cutApplied = (trackFeatures.nHits < getValueAtEta(detector.minHitsPerEta, - etaBins.size(), etaBin)) || - cutApplied; - - cutApplied = (trackFeatures.nHoles > getValueAtEta(detector.maxHolesPerEta, - etaBins.size(), etaBin)) || - cutApplied; - - cutApplied = - (trackFeatures.nOutliers > - getValueAtEta(detector.maxOutliersPerEta, etaBins.size(), etaBin)) || - cutApplied; - - return cutApplied; + return (trackFeatures.nHits < getValueAtEta(detector.minHitsPerEta, + etaBins.size(), etaBin)) || + (trackFeatures.nHoles > getValueAtEta(detector.maxHolesPerEta, + etaBins.size(), etaBin)) || + (trackFeatures.nOutliers > + getValueAtEta(detector.maxOutliersPerEta, etaBins.size(), etaBin));Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp (1)
57-58: Document the minUnshared parameter, you must.Add documentation explaining the purpose and impact of minUnshared parameter, essential it is.
// minimum number of unshared hits per track - std::size_t minUnshared = 5; + /// @brief Minimum number of unshared hits required per track + /// @details Used to filter out tracks with too many shared hits, + /// ensuring track quality and reducing ambiguity + std::size_t minUnshared = 5;Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
74-75: Initialize vectors before size calculation, you should.Move vector initialization after size validation, cleaner code it makes.
- detectorConfig.factorHits = {}; detectorConfig.maxHits = goodHits.size() - 1; + detectorConfig.factorHits.reserve(goodHits.size()); - detectorConfig.factorHoles = {}; detectorConfig.maxHoles = goodHoles.size() - 1; + detectorConfig.factorHoles.reserve(goodHoles.size());Also applies to: 84-85
Examples/Scripts/Python/full_chain_test.py (1)
730-730: Complete the changes with documentation, we should.Properly configured the test chain is, matching other implementations it does. Yet, document the new parameter's purpose and impact, we should.
Add this comment above the configuration:
+ # Configure ambiguity resolution with minUnshared=3 for eta-based track selection addScoreBasedAmbiguityResolution(Examples/Python/python/acts/examples/reconstruction.py (1)
219-223: Approve changes to ScoreBasedAmbiguityResolutionConfig, but documentation needs, hmm!Good addition of
minUnsharedparameter, I see. But document its purpose and usage, we must. Help future Padawans understand the ways of eta-based cuts, this will.Add docstring to explain the parameter:
ScoreBasedAmbiguityResolutionConfig = namedtuple( "ScoreBasedAmbiguityResolutionConfig", [ "minScore", "minScoreSharedTracks", "maxShared", + # Minimum number of unshared measurements required for track selection. + # Can be specified as a single value or as an array corresponding to eta bins. "minUnshared", "maxSharedTracksPerMeasurement", "useAmbiguityFunction", ], defaults=[None] * 6, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp(5 hunks)Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp(11 hunks)Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp(1 hunks)Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp(1 hunks)Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp(1 hunks)Examples/Python/python/acts/examples/reconstruction.py(2 hunks)Examples/Python/src/AmbiguityResolution.cpp(1 hunks)Examples/Scripts/Python/full_chain_odd.py(1 hunks)Examples/Scripts/Python/full_chain_odd_LRT.py(1 hunks)Examples/Scripts/Python/full_chain_test.py(1 hunks)Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(2 hunks)Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: missing_includes
- GitHub Check: docs
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (22)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (3)
41-41: No genuine change in functionality, I sense.
Marked line 41 is, but truly unchanged it appears.
54-61: Eta-based vectors, more granular configuration they allow.
Handle mismatched sizes carefully, you must. Potential out-of-range issues, watchful be.
187-190: Method for eta-based cuts, helpful it can be.
Edge cases for extreme eta or improper bin coverage, verify you should.Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp (14)
110-110: Comment clarifies purpose, small overhead it has.
Substantial gain in readability, offers it does.
118-122: Verbose logging, valuable in debugging it is.
Yet mindful be of performance in large-scale runs.
124-125: Score zeroing upon cut failure, straightforward it is.
Logic consistent with the design, yes.
140-140: Section to commence scoring, well-indicated it is.
Better readability fosters, this does.
226-226: Detector cuts repeated in advanced scoring, I see.
Alignment with prior logic, good it remains.
240-241: Eta-based cuts mirror the simpleScore path, consistent it is.
Harmonious approach for both scoring methods, yes.
256-257: Ambiguity scoring, pT-based advantage it confers.
Sense it makes for higher momentum priority.
274-274: Hit multiplier logic, interesting approach it is.
Safeguards for large nHits or negative, the code suggests. Fine it seems.
308-308: Track index increment, normal step this is.
No further remarks I have.
390-391: Eta retrieved once, reusing best practice it is.
Computational overhead, lowered you have.
395-407: Eta bin determination, loop-based approach.
Should out-of-range conditions occur, fallback logic implement, you should.
422-422: Threshold check for good tracks, straightforward it is.
Concise clarity in naming, you maintain.
537-538: Shared hit acceptance, depends on minScoreSharedTracks and config.
Balanced logic, yes. Works for repeated hits, it does.
550-550: Enforcement of minUnshared hits, consistent with earlier config.
Consider site-specific adjustments, you might.Examples/Python/src/AmbiguityResolution.cpp (1)
36-37: Parameter signature aligned, I see.
Farewell to angular constraints, yes. minUnshared included, a worthy addition this is.Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp (1)
47-47: Correct, the transformation of minUnshared is.Clean and straightforward, the implementation is.
Examples/Scripts/Python/full_chain_odd.py (1)
417-420: Consistent configuration changes, these are.Match the test implementation, these changes do. Proper value for
minUnsharedparameter, set it does. Removed momentum and angular parameters, as intended they are.Examples/Scripts/Python/full_chain_odd_LRT.py (1)
420-420: Maintain consistency across chains, this change does.Mirror the configuration in other files, it does. Proper implementation of eta-based cuts strategy, this is.
Examples/Python/python/acts/examples/reconstruction.py (1)
1920-1922: Strong with the Force, these changes are!Correctly integrated the
minUnsharedparameter is. Proper flow from configuration to algorithm implementation, I sense.
Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
54-78: Repetitive code in eta-based vectors initialization, I observe.A helper function to reduce duplication, we should create.
Apply this refactor, you should:
+ template<typename T> + void initializeEtaVector(std::vector<T>& target, + const std::vector<T>& source, + const std::string& name) { + target = {}; + for (auto value : source) { + target.push_back(value); + } + } - const std::vector<std::size_t>& minHitsPerEta = value["minHitsPerEta"]; - detectorConfig.minHitsPerEta = {}; - for (auto minHit : minHitsPerEta) { - detectorConfig.minHitsPerEta.push_back(minHit); - } + initializeEtaVector(detectorConfig.minHitsPerEta, + value["minHitsPerEta"].get<std::vector<std::size_t>>(), + "minHitsPerEta"); - const std::vector<std::size_t>& maxHolesPerEta = value["maxHolesPerEta"]; - detectorConfig.maxHolesPerEta = {}; - for (auto maxHole : maxHolesPerEta) { - detectorConfig.maxHolesPerEta.push_back(maxHole); - } + initializeEtaVector(detectorConfig.maxHolesPerEta, + value["maxHolesPerEta"].get<std::vector<std::size_t>>(), + "maxHolesPerEta"); - const std::vector<std::size_t>& maxOutliersPerEta = value["maxOutliersPerEta"]; - detectorConfig.maxOutliersPerEta = {}; - for (auto maxOutlier : maxOutliersPerEta) { - detectorConfig.maxOutliersPerEta.push_back(maxOutlier); - } + initializeEtaVector(detectorConfig.maxOutliersPerEta, + value["maxOutliersPerEta"].get<std::vector<std::size_t>>(), + "maxOutliersPerEta"); - const std::vector<std::size_t>& maxSharedHitsPerEta = value["maxSharedHitsPerEta"]; - detectorConfig.maxSharedHitsPerEta = {}; - for (auto maxSharedHit : maxSharedHitsPerEta) { - detectorConfig.maxSharedHitsPerEta.push_back(maxSharedHit); - } + initializeEtaVector(detectorConfig.maxSharedHitsPerEta, + value["maxSharedHitsPerEta"].get<std::vector<std::size_t>>(), + "maxSharedHitsPerEta");Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (2)
194-202: Documentation enhancement for etaBasedCuts method, suggest I do.More details about return value and parameter constraints, helpful would be.
Add these details to the documentation, you should:
/// Rejects Tracks based on eta dependent cuts. /// /// @param track is the input track /// @param trackFeatures is the trackFeatures object for a specific detector /// @param eta is the eta of the track + /// @param detector is the detector configuration containing eta-based cuts + /// @pre eta must fall within the range defined by detector.etaBins /// @return true if the track is rejected, false otherwise + /// @throws std::out_of_range if eta is outside the configured range
240-248: Documentation enhancement for getValueAtEta method, suggest I do.Parameter constraints and error conditions, documented they should be.
Add these details to the documentation, you should:
/// Get the value at a specific eta bin. /// /// @param cuts is the input vector of cuts /// @param etaBinSize is the size of the eta bin /// @param binIndex is the index of the bin + /// @pre cuts vector must not be empty + /// @pre binIndex must be less than etaBinSize /// @return the value of cut at the specific eta bin + /// @throws std::out_of_range if binIndex >= etaBinSize + /// @throws std::invalid_argument if cuts vector is empty
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp(5 hunks)Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp(1 hunks)Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (3)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
84-85: Correct initialization of factor vectors and max values, I see.Well structured and consistent with the pattern, this code is.
Also applies to: 94-95
38-52:⚠️ Potential issueIncomplete validation of eta bins configuration, I sense.
Additional validation checks, we must add:
- After initialization, empty vectors check we should
- Size consistency between eta bins and other eta-based vectors, verify we must
Apply this diff, you should:
const std::vector<double>& etaBins = value["etaBins"]; // Validate eta bins - if (!etaBins.empty()) { + if (etaBins.empty()) { + throw std::invalid_argument("Empty eta bins configuration"); + } + // Verify monotonically increasing eta bins if (!std::is_sorted(etaBins.begin(), etaBins.end())) { throw std::invalid_argument( "Eta bins must be monotonically increasing"); } detectorConfig.etaBins = {}; for (auto etaBin : etaBins) { detectorConfig.etaBins.push_back(etaBin); } + + // Verify size consistency with other eta-based vectors + if (minHitsPerEta.size() != etaBins.size() - 1 || + maxHolesPerEta.size() != etaBins.size() - 1 || + maxOutliersPerEta.size() != etaBins.size() - 1 || + maxSharedHitsPerEta.size() != etaBins.size() - 1) { + throw std::invalid_argument( + "Inconsistent sizes in eta-based configuration vectors"); + } - }Likely invalid or redundant comment.
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (1)
54-68: Well documented and properly initialized eta-based configuration, this is.Clear purpose and sensible defaults, these vectors have.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (3)
194-203: Implementation of eta-based cuts, elegant it is!Clear purpose and parameters, this method has. But documentation, improved it could be.
Add these clarifications to the documentation, you should:
/// Rejects Tracks based on eta dependent cuts. /// + /// Evaluates track features against thresholds defined per eta bin: + /// - Minimum hits requirement + /// - Maximum holes constraint + /// - Maximum outliers limit + /// - Maximum shared hits boundary /// /// @param detector is the detector configuration object /// @param trackFeatures is the trackFeatures object for a specific detector /// @param eta is the eta of the track /// @return true if the track is rejected, false otherwise
240-249: Helper method getValueAtEta, utility strong with this one is!Static helper method, well-defined it is. But parameter names, clearer they could be.
Improve parameter names, we should:
- static std::size_t getValueAtEta(std::vector<std::size_t> cuts, - std::size_t etaBinSize, - std::size_t binIndex); + static std::size_t getValueAtEta(std::vector<std::size_t> thresholds, + std::size_t nEtaBins, + std::size_t currentBinIndex);
41-41: Documentation update needed, it is!Step 5 in algorithm description, outdated it has become. Eta-based cuts, mentioned they should be.
- /// 5) Remove tracks that are not good enough based on cuts Contains method for - /// data preparations + /// 5) Remove tracks that do not meet eta-dependent quality criteria for hits, + /// holes, outliers, and shared hits. Contains methods for data preparation + /// and track selection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp(5 hunks)
🔇 Additional comments (1)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (1)
123-124: Wise addition of minUnshared parameter, it is!Track quality control through unshared hits, essential it is. Default value of 5, reasonable starting point it seems.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
14-20: Simplify the initializeEtaVector function, we should.Unnecessary complexity in vector initialization, I sense. Direct assignment or std::copy, more efficient it would be.
-void initializeEtaVector(std::vector<std::size_t>& target, - const std::vector<std::size_t>& source) { - target = {}; - for (auto value : source) { - target.push_back(value); - } -} +void initializeEtaVector(std::vector<std::size_t>& target, + const std::vector<std::size_t>& source) { + target = source; +}
79-80: Remove unnecessary vector initialization, we should.Empty initialization redundant it is. By default, empty the vectors will be.
- detectorConfig.factorHits = {}; detectorConfig.maxHits = goodHits.size() - 1; - detectorConfig.factorHoles = {}; detectorConfig.maxHoles = goodHoles.size() - 1;Also applies to: 89-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:40:33.697Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing. The eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) can have either size 1 (single value for all bins) or size equal to etaBins.size() - 1 (per-bin values).
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:35:12.939Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing and the size of eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) must be equal to etaBins.size() - 1.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: merge-sentinel
- GitHub Check: build_debug
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
61-73: Correct initialization of eta-based cut vectors, this is.Wise decision to defer size validation to runtime, it is. Size of 1 or etaBins.size() - 1, these vectors may have.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
14-20: Simplify the vector initialization, we should.Unnecessary complexity in vector initialization, I sense. Direct assignment or
std::copy, more elegant it would be.-void initializeEtaVector(std::vector<std::size_t>& target, - const std::vector<std::size_t>& source) { - target = {}; - for (auto value : source) { - target.push_back(value); - } -} +void initializeEtaVector(std::vector<std::size_t>& target, + const std::vector<std::size_t>& source) { + target = source; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (3)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:40:33.697Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing. The eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) can have either size 1 (single value for all bins) or size equal to etaBins.size() - 1 (per-bin values).
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:35:12.939Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing and the size of eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) must be equal to etaBins.size() - 1.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:47-59
Timestamp: 2025-01-29T11:54:30.076Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, empty eta bins in JSON configuration are acceptable as the struct has default values of {-5, 5} for etaBins.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
47-59: Wise validation of eta bins, this is.Correctly implemented, the validation is:
- Empty eta bins, allowed they are (default values {-5, 5} they have)
- Monotonically increasing order, enforced it is
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
14-28: Improve error handling in initializeEtaVector function, hmm.Missing validation of input parameters and integer division in error message could lead to issues, it could.
void initializeEtaVector(std::vector<std::size_t>& target, const std::vector<std::size_t>& source, int etaBinSize) { + if (etaBinSize <= 1) { + throw std::invalid_argument("etaBinSize must be greater than 1"); + } if (source.size() == etaBinSize - 1) { target = source; // Directly copy if sizes match } else if (source.size() == 1) { target.resize(etaBinSize - 1); // Resize target to the required size std::fill(target.begin(), target.end(), source[0]); // Fill with the single value from source } else { - throw std::invalid_argument("Invalid cuts size. Expected 1 or " + - std::to_string(etaBinSize - 1) + ", got " + - std::to_string(source.size())); + throw std::invalid_argument( + fmt::format("Invalid cuts size. Expected 1 or {}, got {}", + etaBinSize - 1, source.size())); } }Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp (1)
396-404: Extract eta bin lookup logic to helper function, you should.Duplicate eta bin lookup logic, I see. In solveAmbiguity and etaBasedCuts, similar code exists. Extract to helper function, we must.
+ int getEtaBin(const std::vector<double>& etaBins, double eta) { + for (std::size_t i = 0; i < etaBins.size() - 1; ++i) { + if (eta >= etaBins[i] && eta < etaBins[i + 1]) { + return i; + } + } + return -1; // Invalid bin + }Use helper function in both places:
- std::vector<double> etaBins = detector.etaBins; - int etaBin = 0; - - for (std::size_t i = 0; i < detector.etaBins.size() - 1; ++i) { - if (eta >= etaBins[i] && eta < detector.etaBins[i + 1]) { - etaBin = i; - break; - } - } + int etaBin = getEtaBin(detector.etaBins, eta); + if (etaBin < 0) { + continue; // Skip if eta is out of range + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp(4 hunks)Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp(10 hunks)Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp(1 hunks)Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (3)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (3)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:40:33.697Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing. The eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) can have either size 1 (single value for all bins) or size equal to etaBins.size() - 1 (per-bin values).
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:35:12.939Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing and the size of eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) must be equal to etaBins.size() - 1.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:47-59
Timestamp: 2025-01-29T11:54:30.076Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, empty eta bins in JSON configuration are acceptable as the struct has default values of {-5, 5} for etaBins.
Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp (1)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp:54-68
Timestamp: 2025-01-29T12:15:02.454Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, the default eta bin configuration [-5,5] is intentionally set as a single bin to disable eta-based functionality by default. The per-eta vectors (minHitsPerEta, maxHolesPerEta, etc.) can have either size=1 (single value for all bins) or size=etaBins.size()-1 (value per bin).
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (1)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp:54-68
Timestamp: 2025-01-29T12:15:02.454Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, the default eta bin configuration [-5,5] is intentionally set as a single bin to disable eta-based functionality by default. The per-eta vectors (minHitsPerEta, maxHolesPerEta, etc.) can have either size=1 (single value for all bins) or size=etaBins.size()-1 (value per bin).
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (1)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp (1)
54-68: Wise default configuration, it is!Default eta bins {-5, 5} with single bin configuration, elegant solution for disabling eta-based functionality by default, it is. Well documented and implemented, the configuration is.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
14-28: Parameter types, improve we must.Inconsistent parameter types, I see. Using
intforetaBinSizewhenstd::size_tfor vectors we use.Apply this change, you should:
void initializeEtaVector(std::vector<std::size_t>& target, const std::vector<std::size_t>& source, - int etaBinSize) { + std::size_t etaBinSize) {
53-67: Simplify vector assignment, we can.Unnecessary loop for vector copying, I see. Direct assignment, more efficient it would be.
Apply this change, you should:
- detectorConfig.etaBins = {}; - for (auto etaBin : etaBins) { - detectorConfig.etaBins.push_back(etaBin); - } + detectorConfig.etaBins = etaBins;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (4)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:40:33.697Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing. The eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) can have either size 1 (single value for all bins) or size equal to etaBins.size() - 1 (per-bin values).
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp:11-24
Timestamp: 2025-01-29T17:44:41.269Z
Learning: In Acts::ScoreBasedAmbiguityResolution, eta and etaBins validation is handled at configuration time in AmbiguityConfigJsonConverter.cpp, which validates that etaBins are non-empty and monotonically increasing. Therefore, additional validation in etaBasedCuts() method is not needed.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:35:12.939Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing and the size of eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) must be equal to etaBins.size() - 1.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:47-59
Timestamp: 2025-01-29T11:54:30.076Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, empty eta bins in JSON configuration are acceptable as the struct has default values of {-5, 5} for etaBins.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (1)
69-85: Well implemented, the eta-based cuts initialization is!Consistent initialization of cut vectors using
initializeEtaVector, I see. Proper error handling and size validation, you have included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp (2)
488-499: Defensive programming, strong with this one is.Proper error handling for volume mapping, you have implemented. But improve error message formatting, we could.
- ACTS_ERROR("Volume " << ivolume << "not found in the volume map"); + ACTS_ERROR("Volume " << ivolume << " not found in the volume map");
Line range hint
545-549: Simplified return logic, recommend I do.More concise and clear, the return statement could be.
- if (newMeasurementsPerTrack.size() < m_cfg.minUnshared) { - return false; - } else { - return true; - } + return newMeasurementsPerTrack.size() >= m_cfg.minUnshared;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (4)
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp (4)
118-122: Improved logging verbosity, I see!More detailed debug information for track features, you have added. Help in debugging and monitoring track quality, this will.
124-125: Refactored to eta-based cuts, the code has been.Moved from individual parameter checks to a unified eta-based approach, you have. More maintainable and aligned with particle physics conventions, this makes the code.
532-533: Potential race condition in shared hits handling, I sense.When multiple tracks processed in parallel are, race condition in shared hits counting may occur. Thread safety considerations, you should add.
Consider adding synchronization mechanisms if parallel track processing is implemented.
396-400: Verify eta binning implementation, we must.Critical for correct track selection, proper eta binning is. Ensure correct bin assignment when eta equals bin boundary value, we should.
✅ Verification successful
Correct and safe, the eta binning implementation is.
Proper handling of boundaries, I sense. When eta equals bin edge, correct bin it finds. Out-of-range values, safely rejected they are. Validation of monotonic ordering, the JSON converter ensures. Trust in this implementation, we can.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases that verify eta binning edge cases rg -A 10 "etaBins.*test"Length of output: 26
Script:
#!/bin/bash # Search for ambiguity resolution test files fd "Ambiguity.*test" --type f # Search for eta binning related code rg -g "!*.git" "etaBins" -A 5 # Look for detector class definition ast-grep --pattern 'class $_ { $$$ etaBins $$$ }'Length of output: 12511
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
14-28: Document the function's purpose and improve error message clarity, we must.Clear documentation, essential it is. And error message, more descriptive it should be.
+/** + * Initialize eta-based vector with either per-bin values or a single value for all bins. + * @param target Vector to be initialized + * @param source Source vector containing either one value or per-bin values + * @param etaBinSize Size of eta bins plus one + * @throws std::invalid_argument if source vector size is invalid + */ void initializeEtaVector(std::vector<std::size_t>& target, const std::vector<std::size_t>& source, std::size_t etaBinSize) { if (source.size() == etaBinSize - 1) { target = source; // Directly copy if sizes match } else if (source.size() == 1) { target.resize(etaBinSize - 1); // Resize target to the required size std::fill(target.begin(), target.end(), source[0]); // Fill with the single value from source } else { throw std::invalid_argument("Invalid cuts size. Expected 1 or " + - std::to_string(etaBinSize - 1) + ", got " + - std::to_string(source.size())); + std::to_string(etaBinSize - 1) + + " (number of eta bin boundaries), got " + + std::to_string(source.size()) + + ". Use size 1 for single value or size " + + std::to_string(etaBinSize - 1) + + " for per-bin values."); } }
53-67: Simplify eta bins assignment, we should.More concise, the code can be. Direct assignment, sufficient it is.
const std::vector<double>& etaBins = value["etaBins"]; // Validate eta bins if (!etaBins.empty()) { // Verify monotonically increasing eta bins if (!std::is_sorted(etaBins.begin(), etaBins.end())) { throw std::invalid_argument( "Eta bins must be monotonically increasing"); } - detectorConfig.etaBins = {}; - for (auto etaBin : etaBins) { - detectorConfig.etaBins.push_back(etaBin); - } + detectorConfig.etaBins = etaBins; // Direct vector assignment }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (4)
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:40:33.697Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing. The eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) can have either size 1 (single value for all bins) or size equal to etaBins.size() - 1 (per-bin values).
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp:11-24
Timestamp: 2025-01-29T17:44:41.269Z
Learning: In Acts::ScoreBasedAmbiguityResolution, eta and etaBins validation is handled at configuration time in AmbiguityConfigJsonConverter.cpp, which validates that etaBins are non-empty and monotonically increasing. Therefore, additional validation in etaBasedCuts() method is not needed.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:0-0
Timestamp: 2025-01-29T10:35:12.939Z
Learning: In the ACTS framework's ambiguity resolution configuration, eta bins must be monotonically increasing and the size of eta-based cut vectors (minHitsPerEta, maxHolesPerEta, maxOutliersPerEta, maxSharedHitsPerEta) must be equal to etaBins.size() - 1.
Learnt from: Ragansu
PR: acts-project/acts#4054
File: Plugins/Json/src/AmbiguityConfigJsonConverter.cpp:47-59
Timestamp: 2025-01-29T11:54:30.076Z
Learning: In Acts::ScoreBasedAmbiguityResolution::DetectorConfig, empty eta bins in JSON configuration are acceptable as the struct has default values of {-5, 5} for etaBins.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (2)
Plugins/Json/src/AmbiguityConfigJsonConverter.cpp (2)
69-86: Well structured, the eta-based cuts initialization is.Proper initialization of all eta-based cuts, I see. Consistent use of
initializeEtaVector, there is.
91-92: Guard against invalid factor calculations, we must.Division by zero, dangerous it is. Negative values, meaningless ratios they create.
Also applies to: 101-102
|
Introduced eta-based cuts in ACTS Standalone.
--- END COMMIT MESSAGE ---
To bring the ScoreBased solver closer to that of ATHENA, I have introduced eta-based cuts in ACTS Standalone. Previously eta-based cuts required in ATHENA were done using optional cuts. With this PR, all detector-level cuts are now eta-based and the cuts can be a single number or an array with the size of the eta bins. A new config file is required for this change and hence I updated Tests, Examples, python scripts, Json plugins
Thanks @Corentin-Allaire and @CarloVarni
Summary by CodeRabbit
New Features
minUnsharedfor track ambiguity resolution.Refactor
Improvements