feat!: Updates to GBTS for Athena Implementation #3882
Conversation
|
Warning Rate limit exceeded@Rosie-Hasan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 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)
WalkthroughSignificant modifications made, yes. The Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
paulgessinger
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: 0
🧹 Outside diff range and nitpick comments (7)
Core/include/Acts/Seeding/GbtsDataStorage.hpp (1)
Line range hint
223-230: Simplify determination ofisPixel, you can.Consider streamlining the assignment of
isPixel.Refactored code:
- bool isPixel = false; - if (sourceLink.size() == 1) { // pixels have 1 SL - isPixel = true; - } else { - isPixel = false; - } + bool isPixel = (sourceLink.size() == 1); // pixels have 1 SLExamples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)
223-230: SimplifyisPixeldetermination, you should.Streamlining the code improves readability.
Simplify as follows:
- bool isPixel = false; - if (sourceLink.size() == 1) { // pixels have 1 SL - isPixel = true; - } else { - isPixel = false; - } + bool isPixel = (sourceLink.size() == 1); // pixels have 1 SLCore/include/Acts/Seeding/SeedFinderGbtsConfig.hpp (1)
46-46: Hmmmm, good the naming change is. Documentation, we must add.The renaming to PascalCase convention, consistent with other changes in the codebase it is. Yet, a comment explaining the purpose of this configuration file, helpful it would be.
Add a documentation comment like this:
+ // Path to the connector configuration file that defines the layer connections std::string ConnectorInputFile; // input file for connector objectCore/include/Acts/Seeding/SeedFinderGbts.ipp (1)
168-172: Hmmmm, simplified access patterns, good they are. Yet optimize further, we can.The direct access to space point properties through
m_spGbts, cleaner code it creates. However, for performance in critical calculations, consider caching frequently accessed values:- float r1 = n1->m_spGbts.r(); - float x1 = n1->m_spGbts.SP->x(); - float y1 = n1->m_spGbts.SP->y(); - float z1 = n1->m_spGbts.SP->z(); - float phi1 = n1->m_spGbts.phi(); + const auto& sp = n1->m_spGbts; + const float r1 = sp.r(); + const float x1 = sp.SP->x(); + const float y1 = sp.SP->y(); + const float z1 = sp.SP->z(); + const float phi1 = sp.phi();Also applies to: 201-201, 547-547
Examples/Python/python/acts/examples/reconstruction.py (3)
281-281: Parameter naming convention updated, yes.Renamed parameter follows Python naming conventions better it does not. PascalCase for parameters unusual in Python it is. Consider snake_case instead.
- ConnectorInputConfigFile: Optional[Union[Path, str]] = None, + connector_input_config_file: Optional[Union[Path, str]] = None,
437-437: Consistent parameter renaming applied, but naming convention concerns persist.Changed for consistency with parameter definition it was, but same naming convention issue exists.
- ConnectorInputConfigFile, + connector_input_config_file,
1094-1107: Cluster width feature disabled by default, hmm.New infrastructure for cluster width added it was, but disabled by default it remains. Matches PR objectives this does, which mention feature is currently disabled in examples.
Two observations I have:
- Parameter naming inconsistency:
- ConnectorInputFileStr = str(ConnectorInputConfigFile) + connector_input_file_str = str(connector_input_config_file)
- Documentation for new parameter
m_useClusterWidthmissing it is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Core/include/Acts/Seeding/GbtsDataStorage.hpp(4 hunks)Core/include/Acts/Seeding/GbtsTrackingFilter.hpp(3 hunks)Core/include/Acts/Seeding/SeedFinderGbts.ipp(3 hunks)Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/GbtsSeedingAlgorithm.hpp(0 hunks)Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp(3 hunks)Examples/Python/python/acts/examples/reconstruction.py(3 hunks)Examples/Python/src/TrackFinding.cpp(2 hunks)Examples/Scripts/Python/full_chain_itk_Gbts.py(1 hunks)
💤 Files with no reviewable changes (1)
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/GbtsSeedingAlgorithm.hpp
🔇 Additional comments (16)
Core/include/Acts/Seeding/GbtsDataStorage.hpp (8)
32-35: New member variables added, helpful they are.
Adding m_isPixel, m_phi, m_r, and m_ClusterWidth enhances data encapsulation within GbtsSP.
36-44: Constructor updated appropriately, it is.
The constructor initializes new members and computes m_phi and m_r correctly.
49-50: Getter methods provided, good practice this is.
Implementing accessors for r() and ClusterWidth() ensures proper data encapsulation.
160-160: Ensure consistency of sp.r(), you must.
Verify that sp.r() aligns with m_r, to maintain data consistency.
173-173: Cluster width retrieval, appropriate it is.
Using sp.ClusterWidth() to obtain the cluster width is correct.
184-184: Cluster width check, necessary it is.
Validating cluster_width > 0.2 maintains data integrity.
Line range hint 232-233: Placeholder for ClusterWidth, mindful you must be.
Setting ClusterWidth to zero may impact calculations. Update when actual values available, you should.
Line range hint 235-236: Constructor call updated correctly, it is.
Including new parameters in gbtsSpacePoints.emplace_back properly done.
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (4)
70-70: Input file stream initialized, acceptable it is.
input_ifstream correctly opens ConnectorInputFile from configuration.
89-97: Debug logging enhanced, clarity improved.
Logging of Gbts space points provides valuable information for debugging.
232-233: ClusterWidth set to zero, verify implications you must.
Ensure that a zero ClusterWidth does not adversely affect computations.
235-236: Constructor call updated, parameters included properly.
gbtsSpacePoints.emplace_back now correctly includes isPixel and ClusterWidth.
Examples/Python/src/TrackFinding.cpp (1)
206-206: Parameter name updated, consistency maintained.
Renaming connector_input_file to ConnectorInputFile aligns with naming conventions.
Examples/Scripts/Python/full_chain_itk_Gbts.py (1)
98-98: Parameter name updated, consistency improved it is.
Changed connector_inputConfigFile to ConnectorInputConfigFile, reflecting updates in configuration naming.
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp (1)
55-56: Simplified access to space point properties, I sense. Wise decision, this is.
Direct access to r() through m_spGbts instead of m_spGbts.SP, cleaner and more maintainable the code becomes. Consistent with changes throughout the codebase, this pattern is.
Verify the consistency of this pattern, we must:
Also applies to: 70-70, 279-279
✅ Verification successful
Let me search for these patterns in different ways to verify the consistency, hmm. The Force will guide us to the truth about these access patterns, it will.
Consistent the changes are, throughout the codebase they flow
Through the Force, seen I have that both patterns coexist:
- Direct access through
m_spGbts.r()for radius - Legacy access through
m_spGbts.SP->x(),m_spGbts.SP->y(),m_spGbts.SP->z()
A transition in progress this appears to be, where radius access simplified is, while other properties through SP pointer still flow. Natural this evolution is, as step by step the code transforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old-style access patterns
rg -A 2 'm_spGbts\.SP->[r|phi]\(\)'
Length of output: 37
Script:
#!/bin/bash
# Search for any occurrences of m_spGbts to understand the access pattern
rg -A 2 'm_spGbts'
# Also search for specific radius access patterns
rg -A 2 '->r\(\)'
# And search for phi access patterns
rg -A 2 '->phi\(\)'
Length of output: 6863
Examples/Python/python/acts/examples/reconstruction.py (1)
1094-1107: Verify the impact of disabled cluster width feature.
Understand the effects of disabled cluster width feature we must.
✅ Verification successful
Safe to disable cluster width feature, it is
Investigated the Force surrounding cluster width, I have. Clear evidence found in the codebase:
- Optional feature it is, defaulting to false in
SeedFinderGbtsConfig.hpp - Used only in space point addition, impact minimal it has
- No critical dependencies or tests broken by disabling, found there were not
Proceed with m_useClusterWidth=False, you may. No disturbance in the Force, this change will cause.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cluster width usage in the codebase
echo "Searching for cluster width usage..."
rg -l "useClusterWidth|m_useClusterWidth"
# Search for related configuration settings
echo "Searching for related configuration..."
rg "cluster.*width.*config" -A 3
Length of output: 482
Script:
#!/bin/bash
# Search for the implementation details of cluster width usage
echo "Searching in SeedFinderGbts.ipp..."
rg -A 10 "m_useClusterWidth" "Core/include/Acts/Seeding/SeedFinderGbts.ipp"
echo "Searching in SeedFinderGbtsConfig.hpp..."
rg -A 5 "m_useClusterWidth" "Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp"
# Check if there are any tests related to cluster width
echo "Searching for tests..."
rg -A 5 "useClusterWidth.*test"
# Check Python bindings for cluster width
echo "Checking Python bindings..."
rg -A 5 "useClusterWidth" "Examples/Python/src/TrackFinding.cpp"
Length of output: 1233
Changing isSCT->isStrip
Co-authored-by: Tim Adye <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/Seeding/GbtsDataStorage.hpp (2)
32-50: Approve the changes to GbtsSP struct, but suggest documentation, hmm.Wise changes these are, using atan2 instead of atan. More robust handling of quadrants, it provides. But documentation, it lacks. Help future padawans understand the purpose of these members, we must.
Add documentation for the new members, you should:
+ /// @brief Indicates if the space point is from a pixel detector bool m_isPixel; + /// @brief Azimuthal angle in the global coordinate system float m_phi; + /// @brief Radial distance from the origin in the global coordinate system float m_r; + /// @brief Width of the cluster in the detector float m_ClusterWidth;
Line range hint
173-177: Extract magic numbers as named constants, we must.Magic numbers in the code, clarity they lack. Constants with meaningful names, better understanding they bring.
Extract these as named constants at class or namespace level:
+ // Cluster width constants for barrel layers + static constexpr float MIN_TAU_SCALE = 6.7f; + static constexpr float MIN_CLUSTER_WIDTH_OFFSET = 0.2f; + static constexpr float MAX_TAU_BASE = 1.6f; + static constexpr float MAX_TAU_FACTOR = 0.15f; + static constexpr float MAX_TAU_SCALE = 6.1f; if (useClusterWidth) { float cluster_width = sp.ClusterWidth(); - min_tau = 6.7 * (cluster_width - 0.2); - max_tau = 1.6 + 0.15 / (cluster_width + 0.2) + 6.1 * (cluster_width - 0.2); + min_tau = MIN_TAU_SCALE * (cluster_width - MIN_CLUSTER_WIDTH_OFFSET); + max_tau = MAX_TAU_BASE + MAX_TAU_FACTOR / (cluster_width + MIN_CLUSTER_WIDTH_OFFSET) + + MAX_TAU_SCALE * (cluster_width - MIN_CLUSTER_WIDTH_OFFSET);Also applies to: 184-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Seeding/GbtsDataStorage.hpp(4 hunks)Core/include/Acts/Seeding/SeedFinderGbts.ipp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Seeding/SeedFinderGbts.ipp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: fpe_masks
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (1)
Core/include/Acts/Seeding/GbtsDataStorage.hpp (1)
47-47: Consider renaming isStrip method, a better name we seek.From past wisdom shared by timadye, less ATLAS-specific name we should use, as only in LRT mode this is used.
Suggestion from code rabbit
Comment suggested by code rabbit
Fixing format of comment
removing duplicate definition
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (2)
70-70: Safer file path handling, implement you must.Check file existence before opening, and std::filesystem use, you should. Prevent undefined behavior when file exists not, this will.
- m_cfg.seedFinderConfig.ConnectorInputFile.c_str(), std::ifstream::in); + std::filesystem::path(m_cfg.seedFinderConfig.ConnectorInputFile), std::ifstream::in); + if (!input_ifstream.is_open()) { + throw std::runtime_error("Failed to open connector input file"); + }
Line range hint
79-85: Magic numbers in layer identification, replace we must.Constants or enums for layer identification ranges use, you should. More maintainable and less error-prone, the code will be.
+enum class LayerType { + BARREL_MIN = 79, + BARREL_MAX = 85, + POSITIVE_MIN = 89, + POSITIVE_MAX = 99 +}; -if (79 < Gbts_id && Gbts_id < 85) { // 80s, barrel +if (static_cast<int>(LayerType::BARREL_MIN) < Gbts_id && + Gbts_id < static_cast<int>(LayerType::BARREL_MAX)) {Also applies to: 89-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (1)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)
223-231: Handle cluster width properly, we must.Hardcoded cluster width of 0, concerning it is. Document impact of this limitation and plan for future implementation, we should.
Run this verification to check cluster width usage:
There was a problem hiding this comment.
There are only a few changes in this PR, and I’ve left a few comments for consideration.
That said, I’ve noticed that this version of the code could benefit from aligning more closely with the requirements for deployment in ACTS. Specifically, the code could benefit from additional testing and documentation. This is also reflected in the SonarCloud analysis: https://sonarcloud.io/component_measures?id=acts-project_acts.
While these points aren’t within the scope of this PR, they might be worth addressing in future updates to the code. It could be a good opportunity to incorporate some of these improvements as part of our planned updates.
| bool isPixel() const { return m_isPixel; } | ||
| bool isSCT() const { return !m_isPixel; } | ||
| bool isStrip() const { return !m_isPixel; } |
There was a problem hiding this comment.
This looks a lot detector dependent. Not for now, but probably we need to define a strategy to generalise this code.
| // In normal (non LRT) mode penalise SSS by 1000, PSS (if enabled) and | ||
| // PPS by 10000 |
There was a problem hiding this comment.
I think we can neglect the LRT case for the moment. Also, I do not think we build PSS and/or PPS at all, so we can probably remove those cases?
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Seeding/GbtsDataStorage.hpp(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (3)
Core/include/Acts/Seeding/GbtsDataStorage.hpp (3)
30-36: Document the member variables, young padawan must.Missing documentation for member variables, I see. Clarity in code, essential it is.
Add documentation for:
m_isPixel: Purpose of pixel/strip differentiationm_phi: Units and coordinate systemm_r: Units and what it representsm_ClusterWidth: Units and significance
37-45: Precalculate phi and r in constructor, wise choice it is.Good practice to compute
m_phiandm_ronce in constructor, it is. Memory-computation tradeoff, balanced well it has been.
47-51: Detector-specific code, concerning this is.Too coupled to specific detector technology (pixel/strip), this implementation is. More generic approach, consider we must.
Run this command to assess impact:
| @@ -152,7 +158,7 @@ class GbtsDataStorage { | |||
| return -1; | |||
| } | |||
|
|
|||
| int binIndex = pL->getEtaBin(sp.SP->z(), sp.SP->r()); | |||
| int binIndex = pL->getEtaBin(sp.SP->z(), sp.r()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error codes, document we must. Return values, explain we should.
Return values (-1, -2, -3), their meaning unclear it is. Documentation for error conditions, add you must.
Add enum for error codes:
+ enum class SpacePointError {
+ Success = 0,
+ InvalidLayer = -1,
+ InvalidEtaBin = -2,
+ InvalidClusterWidth = -3
+ };Also applies to: 185-189
| float cluster_width = sp.ClusterWidth(); | ||
| min_tau = 6.7 * (cluster_width - 0.2); | ||
| max_tau = | ||
| 1.6 + 0.15 / (cluster_width + 0.2) + 6.1 * (cluster_width - 0.2); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Magic numbers in tau calculations, explain you must.
Constants 6.7, 0.2, 1.6, 0.15, and 6.1, mysterious they are. Document their significance and origin, you should.
Extract as named constants with documentation:
+ // Tau calculation constants for barrel layers
+ static constexpr float MIN_TAU_FACTOR = 6.7f;
+ static constexpr float CLUSTER_WIDTH_OFFSET = 0.2f;
+ static constexpr float MAX_TAU_BASE = 1.6f;
+ static constexpr float MAX_TAU_FACTOR1 = 0.15f;
+ static constexpr float MAX_TAU_FACTOR2 = 6.1f;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)
230-231: Document the temporary limitation, you must.Add comment explaining why cluster width is not available in examples and when it will be implemented, you should.
- float ClusterWidth = - 0; // false input as this is not available in examples + float ClusterWidth = 0; + // TODO: Cluster width information will be available when Athena + // implementation is complete. Currently initialized to 0 as placeholder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: macos
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: docs
🔇 Additional comments (2)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (2)
89-101: Safe access patterns implemented, they are. Approve this change, I do.Previous wisdom shared, heeded it was. Source links and geometry ID checks added, preventing crashes they do.
226-234: Incomplete implementation of cluster width, I sense.Initialize cluster width to zero, you do, but use it, you do not. A disturbance in the Force, this creates.
Run this command to verify cluster width usage across the codebase:
Additionally, verify pixel detection logic:
Changes to GBTS seeding in Core and Examples needed to implement in Athena and make it work with any space point type.
--- END COMMIT MESSAGE ---
I have changed the space point class so I believe it will be breaking to Athena infrastructure (draft merge request of patch in canary)
And added infrastructure to use cluster width information, but this is disabled in the examples case
Also removed some unneeded parameters
Summary by CodeRabbit
Release Notes
New Features
GbtsSPstruct with additional parameters for improved spatial information.SeedFinderGbtsandGbtsTrackingFilterclasses.Bug Fixes
Gbts_idassignment in geometry mapping.Documentation
Chores