chore: remove ProtoBinning in favour of ProtoAxis
#4046
Conversation
|
Warning Rate limit exceeded@asalzburger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 4 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)
WalkthroughIn this extensive pull request, the ACTS library undergoes a significant transformation, replacing the Changes
Poem
Possibly related PRs
Suggested Labels
Suggested Reviewers
Sequence DiagramsequenceDiagram
participant OldSystem as ProtoBinning
participant NewSystem as ProtoAxis
participant Detector as Detector Components
OldSystem->>NewSystem: Transform Binning Representation
NewSystem-->>Detector: Updated Axis Configuration
Detector->>NewSystem: Use Enhanced Axis Methods
Hmm, a transformation this is! Refactoring the binning system, we have. More flexible and powerful, the 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
asalzburger
changed the title
ProtoBinning in favour of ProtoAxis
![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 (16)
Core/src/Detector/detail/ProtoMaterialHelper.cpp (1)
23-30: Elegant handling of ProtoAxis, this is.Wise use of modern C++ features, I sense. The emplace_back instead of push_back, more efficient it is. The autorange handling, clear and purposeful it remains.
Consider this small improvement, you might:
- ProtoAxis fBinning(b); - // Check if the binning needs to be fixed - if (fBinning.isAutorange()) { - auto range = sExtent.range(b.getAxisDirection()); - fBinning.setRange(range.min(), range.max()); - } - fbDescription.emplace_back(fBinning); + if (b.isAutorange()) { + auto range = sExtent.range(b.getAxisDirection()); + fbDescription.emplace_back(ProtoAxis(b).setRange(range.min(), range.max())); + } else { + fbDescription.emplace_back(b); + }Plugins/GeoModel/src/detail/GeoModelBinningHelper.cpp (1)
15-15: Renamed well, this method is. Documentation update, suggest I do.Changed from
toProtoBinningtotoProtoAxis, the method name has. Clear the intention is, but update the documentation we should.Add this documentation above the method:
+/// @brief Converts a binning string to a ProtoAxis +/// @param binning The binning string to convert +/// @param extent Optional extent to constrain the binning +/// @return A ProtoAxis object representing the binningAlso applies to: 77-79
Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp (1)
Line range hint
1-1: Architectural wisdom, share I must.A significant transition from ProtoBinning to ProtoAxis, this pull request undertakes. While well-implemented the changes are, consider these steps for a smooth transition:
- Document the migration guide for downstream users, you should
- Update the examples in documentation, you must
- Consider deprecation period for ProtoBinning, wise it would be
- Migration scripts for automated updates, helpful they could be
Plugins/DD4hep/src/DD4hepLayerStructure.cpp (1)
71-79: Strong with the Force, this refactoring is. But handle errors, we must.Properly encapsulated, the binning properties now are. Yet add error handling for invalid range values, we should.
- b.setRange(extent.min(b.getAxisDirection()), - extent.max(b.getAxisDirection())); + auto minVal = extent.min(b.getAxisDirection()); + auto maxVal = extent.max(b.getAxisDirection()); + if (minVal >= maxVal) { + throw std::runtime_error("Invalid extent range: min >= max"); + } + b.setRange(minVal, maxVal);Core/src/Detector/MultiWireStructureBuilder.cpp (1)
82-87: Consistent with the Force, these changes are. But validate we must.The transition to ProtoAxis methods is good, but validate bin edges we should.
+ // Validate bin edges + if (m_cfg.binning[0u].getAxis().getBinEdges().empty() || + m_cfg.binning[1u].getAxis().getBinEdges().empty()) { + throw std::runtime_error("Empty bin edges detected"); + } {m_cfg.binning[0u].getAxis().getBinEdges().front(), m_cfg.binning[0u].getAxis().getBinEdges().back()},Core/src/Utilities/ProtoAxis.cpp (2)
106-127: Improve readability of range setting logic, we must!Better variable names and documentation, suggest I do:
- auto [first, last] = std::ranges::remove_if( - edges, - [minE, maxE](double e) -> bool { return (e < minE || e > maxE); }); + // Find edges that fall outside the new range + auto [outOfRangeStart, outOfRangeEnd] = std::ranges::remove_if( + edges, + [newMin = minE, newMax = maxE](double edge) -> bool { + return (edge < newMin || edge > newMax); + });
168-171: Remove extra semicolon, we shall!A disturbance in the Force, I sense. An extra semicolon at line 170, there is.
- os << a.toString() << '\n'; - ; + os << a.toString() << '\n';Core/include/Acts/Utilities/ProtoAxis.hpp (1)
86-96: More explicit about exceptions, the documentation should be!Add documentation about exceptions that may be thrown, suggest I do:
/// Set the range, in case of autorange, this /// will toggle the autorange flag to false /// - /// @throws an exception if minE > maxE + /// @throws std::invalid_argument if minE > maxE + /// @throws std::runtime_error if the axis is nullCore/src/Geometry/MaterialDesignatorBlueprintNode.cpp (2)
59-67: More descriptive error messages, we need!Improve error messages to guide the Padawan better:
- ACTS_ERROR(prefix() << "Binning is not in RPhi"); - throw std::runtime_error("Binning is not in RPhi"); + ACTS_ERROR(prefix() << "Expected RPhi binning but got " + << loc0.getAxisDirection() << " for outer/inner cylinder"); + throw std::runtime_error("Invalid axis direction for cylinder binning");
166-169: String formatting, improve we must!Use string formatting for cleaner code:
- ss << ": " << loc0.getAxisDirection() << "=" - << loc0.getAxis().getNBins(); - ss << ", " << loc1.getAxisDirection() << "=" - << loc1.getAxis().getNBins(); + ss << std::format(": {}={}, {}={}", + loc0.getAxisDirection(), loc0.getAxis().getNBins(), + loc1.getAxisDirection(), loc1.getAxis().getNBins());Core/include/Acts/Utilities/BinUtility.hpp (1)
116-127: Optimize the vector operations, we should!Pre-allocate capacity for m_binningData based on pAxes size, we must. Prevent multiple reallocations, this will.
BinUtility(const std::vector<ProtoAxis>& pAxes) : m_binningData(), m_transform(Transform3::Identity()), m_itransform(Transform3::Identity()) { m_binningData.reserve(3); + m_binningData.reserve(std::min(pAxes.size(), size_t(3))); for (const auto& pAxis : pAxes) { m_binningData.push_back(BinningData(pAxis)); } }Tests/UnitTests/Core/Detector/CylindricalContainerBuilderTests.cpp (1)
Line range hint
234-238: Consistent formatting, maintain we must!Align the curly braces and parameters consistently, we should. Readability, it improves.
- Acts::ProtoAxis{Acts::AxisDirection::AxisZ, Acts::AxisBoundaryType::Bound, - -480., 480., 14u, 1u}, - Acts::ProtoAxis(Acts::AxisDirection::AxisPhi, - Acts::AxisBoundaryType::Closed, -std::numbers::pi, - std::numbers::pi, 32u, 1u)}; + Acts::ProtoAxis{Acts::AxisDirection::AxisZ, Acts::AxisBoundaryType::Bound, -480., 480., 14u, 1u}, + Acts::ProtoAxis{Acts::AxisDirection::AxisPhi, Acts::AxisBoundaryType::Closed, -std::numbers::pi, std::numbers::pi, 32u, 1u}};Plugins/DD4hep/src/DD4hepBlueprintFactory.cpp (2)
122-125: Fix the typo in variable name, we must!From 'pmProtoAxiss' to 'pmProtoAxes', rename the variable we should. Plural of 'Axis' is 'Axes', remember we must.
- auto pmProtoAxiss = DD4hepBinningHelpers::convertBinning( + auto pmProtoAxes = DD4hepBinningHelpers::convertBinning( dd4hepElement, pmName + "_binning"); current->portalMaterialBinning[p] = - std::vector<ProtoAxis>{pmProtoAxiss}; + std::vector<ProtoAxis>{pmProtoAxes};
126-127: More descriptive debug output, provide we should!Include the portal number and binning details in the debug message, we must.
- ACTS_VERBOSE(ofs << " - binning description is " - << current->portalMaterialBinning[p]); + ACTS_VERBOSE(ofs << " - portal " << p << " material binning: " + << "size=" << current->portalMaterialBinning[p].size() + << ", axes=" << current->portalMaterialBinning[p]);Plugins/DD4hep/src/DD4hepDetectorSurfaceFactory.cpp (1)
158-160: Strong with the Force, these changes are!Wisely have you transitioned to
std::vector<ProtoAxis>. More flexible and clearer, the new implementation is. Yet, a small suggestion I have:Consider consolidating the vector creation on line 221 to avoid unnecessary copy:
- portalMaterialBinning[portalNumber] = - std::vector<ProtoAxis>{protoBinnings}; + portalMaterialBinning[portalNumber] = std::move(protoBinnings);Also applies to: 218-218, 221-221
Core/include/Acts/Utilities/BinningData.hpp (1)
178-199: Constructor implementation, approve I do.Wise implementation, this is. Correctly maps ProtoAxis properties to BinningData members, it does. Handle memory with care, it does. Input validation in IAxis::create already exists, as learned from past encounters we have.
A suggestion for readability, I have:
- const auto& axis = pAxis.getAxis(); + // Get the underlying axis for better readability + const auto& axis = pAxis.getAxis();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
Core/include/Acts/Detector/Blueprint.hpp(2 hunks)Core/include/Acts/Detector/CylindricalContainerBuilder.hpp(2 hunks)Core/include/Acts/Detector/DetectorVolumeBuilder.hpp(2 hunks)Core/include/Acts/Detector/LayerStructureBuilder.hpp(2 hunks)Core/include/Acts/Detector/MultiWireStructureBuilder.hpp(2 hunks)Core/include/Acts/Detector/ProtoBinning.hpp(0 hunks)Core/include/Acts/Detector/ProtoSupport.hpp(2 hunks)Core/include/Acts/Detector/detail/ProtoMaterialHelper.hpp(2 hunks)Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp(3 hunks)Core/include/Acts/Material/ProtoSurfaceMaterial.hpp(3 hunks)Core/include/Acts/Utilities/BinUtility.hpp(2 hunks)Core/include/Acts/Utilities/BinningData.hpp(2 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(6 hunks)Core/src/Detector/CylindricalContainerBuilder.cpp(1 hunks)Core/src/Detector/DetectorVolumeBuilder.cpp(1 hunks)Core/src/Detector/LayerStructureBuilder.cpp(4 hunks)Core/src/Detector/MultiWireStructureBuilder.cpp(3 hunks)Core/src/Detector/detail/ProtoMaterialHelper.cpp(1 hunks)Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp(4 hunks)Core/src/Material/BinnedSurfaceMaterialAccumulater.cpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(3 hunks)Examples/Python/python/acts/examples/detector.py(2 hunks)Examples/Python/python/acts/examples/odd_light.py(6 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepBinningHelpers.hpp(2 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp(2 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerStructure.hpp(1 hunks)Plugins/DD4hep/src/DD4hepBinningHelpers.cpp(4 hunks)Plugins/DD4hep/src/DD4hepBlueprintFactory.cpp(1 hunks)Plugins/DD4hep/src/DD4hepDetectorSurfaceFactory.cpp(1 hunks)Plugins/DD4hep/src/DD4hepLayerStructure.cpp(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoModelBinningHelper.hpp(2 hunks)Plugins/GeoModel/src/GeoModelBlueprintCreater.cpp(3 hunks)Plugins/GeoModel/src/detail/GeoModelBinningHelper.cpp(2 hunks)Tests/UnitTests/Core/Detector/CMakeLists.txt(0 hunks)Tests/UnitTests/Core/Detector/CuboidalContainerBuilderTests.cpp(1 hunks)Tests/UnitTests/Core/Detector/CylindricalContainerBuilderTests.cpp(2 hunks)Tests/UnitTests/Core/Detector/DetectorVolumeBuilderTests.cpp(1 hunks)Tests/UnitTests/Core/Detector/LayerStructureBuilderTests.cpp(2 hunks)Tests/UnitTests/Core/Detector/MultiWireStructureBuilderTests.cpp(1 hunks)Tests/UnitTests/Core/Detector/ProtoBinningTests.cpp(0 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(2 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(3 hunks)Tests/UnitTests/Core/Navigation/MultiWireNavigationTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(3 hunks)Tests/UnitTests/Plugins/ActSVG/DetectorVolumeSvgConverterTests.cpp(2 hunks)Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp(2 hunks)Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- Core/include/Acts/Detector/ProtoBinning.hpp
- Tests/UnitTests/Core/Detector/ProtoBinningTests.cpp
- Tests/UnitTests/Core/Detector/CMakeLists.txt
✅ Files skipped from review due to trivial changes (4)
- Tests/UnitTests/Core/Detector/CuboidalContainerBuilderTests.cpp
- Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerStructure.hpp
- Core/src/Detector/DetectorVolumeBuilder.cpp
- Core/src/Detector/CylindricalContainerBuilder.cpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/Utilities/BinningData.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (46)
Core/src/Material/BinnedSurfaceMaterialAccumulater.cpp (1)
56-56: Hmmmm, approve this change, I do.A wise transition from
toBinUtility()to direct construction, this is. Align with our mission to phase outProtoBinningin favor ofProtoAxis, it does.Verify the constructor's behavior with
ProtoAxis, we must. Run this script, you shall:✅ Verification successful
Safe and wise, this change is, hmmmm.
Through the Force, seen the evidence I have:
- In tests, direct construction of BinUtility from binning data, successful it is
- In implementation, designed for such conversion, the BinUtility class was
- Throughout the codebase, this pattern, well-established it is
Part of our journey to enlightenment through refactoring, this change is. Trust in the Force, we must.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BinUtility constructor implementations and tests echo "=== BinUtility Constructor Implementation ===" rg -A 5 "BinUtility::\s*BinUtility.*ProtoAxis" echo -e "\n=== BinUtility Tests ===" rg -A 5 "TEST.*BinUtility.*ProtoAxis"Length of output: 254
Script:
#!/bin/bash # Search for BinUtility constructors and related components echo "=== BinUtility Constructors ===" ast-grep --pattern 'BinUtility::BinUtility($$$)' echo -e "\n=== binning() Method Implementation ===" rg -A 5 "binning\(\)\s*(const)?\s*{" echo -e "\n=== BinUtility Tests ===" fd "Test" -e cpp -e hpp | xargs rg -l "BinUtility" echo -e "\n=== ProtoGridSurfaceMaterial Implementation ===" ast-grep --pattern 'class ProtoGridSurfaceMaterial { $$$ binning($$$) { $$$ } $$$ }'Length of output: 1863
Script:
#!/bin/bash echo "=== BinUtility Constructor in Tests ===" rg -A 5 "BinUtility\(" Tests/UnitTests/Core/Material/BinnedSurfaceMaterialTests.cpp Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp echo -e "\n=== BinUtility Implementation ===" fd "BinUtility" -e hpp | xargs rg -A 5 "class BinUtility" echo -e "\n=== ProtoGridSurfaceMaterial binning() ===" fd "ProtoGridSurfaceMaterial" -e hpp | xargs rg -A 5 "binning\(\)"Length of output: 7295
Core/include/Acts/Detector/detail/ProtoMaterialHelper.hpp (2)
12-12: Include necessary header, you have.Adding
"Acts/Utilities/ProtoAxis.hpp"appropriate it is, forProtoAxisusage.
31-33: Function signature updated correctly, it is.Changing to
std::vector<ProtoAxis>aligns with the new binning structure.Core/include/Acts/Detector/Blueprint.hpp (2)
16-16: IncludeProtoAxisheader, you must.Including
"Acts/Utilities/ProtoAxis.hpp"ensures proper declaration ofProtoAxis.
102-102: UpdateportalMaterialBinning, well you have.Changing to
std::map<unsigned int, std::vector<ProtoAxis>>reflects the new structure.Core/src/Detector/LayerStructureBuilder.cpp (2)
26-26: IncludeProtoAxis, appropriate it is.Including
"Acts/Utilities/ProtoAxis.hpp"necessary forProtoAxisusage, it is.
223-234: Ensure binning configurations valid are.After sorting
binnings, verify their consistency before creating the updater, we must. SincecreateUpdaterrelies on them, essential it is.Core/src/Detector/detail/ProtoMaterialHelper.cpp (1)
15-18: Approve the signature change, I do.Clean and modern, this signature change is. From BinningDescription to vector, a wise transition it represents, hmm.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoModelBinningHelper.hpp (1)
47-49: Clear and concise, the new function declaration is.From ProtoBinning to ProtoAxis, the journey continues. Well-documented and properly aligned with the new architecture, it is.
Core/include/Acts/Detector/MultiWireStructureBuilder.hpp (1)
44-44: Simple yet powerful, this change is.From ProtoBinning to ProtoAxis, the Config struct evolves. Maintain backward compatibility while moving forward, it does.
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepBinningHelpers.hpp (1)
81-87: Well-documented and clear, the interface change is.From DD4hep to Acts ProtoAxis, the conversion path remains clear. Documentation updated properly, it has been. Interface simplified, yet powerful it remains.
Verify the impact on DD4hep integration, we must.
✅ Verification successful
Complete and consistent, the ProtoAxis migration is.
Successfully migrated, all DD4hep components have been. No traces of old ProtoBinning remain, while ProtoAxis properly integrated it is. In balance with the Force, your changes are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ProtoBinning in DD4hep related files echo "Searching for old ProtoBinning references in DD4hep files..." rg -l "ProtoBinning" --type cpp --type hpp | grep -i "dd4hep"Length of output: 218
Script:
#!/bin/bash # Search with correct file type specifications echo "=== Searching for ProtoBinning references ===" rg -t cpp -t h "ProtoBinning" ./Plugins/DD4hep echo -e "\n=== Searching for ProtoAxis usage ===" rg -t cpp -t h "ProtoAxis" ./Plugins/DD4hep echo -e "\n=== Checking all DD4hep plugin files ===" find ./Plugins/DD4hep -type f -name "*.cpp" -o -name "*.hpp" -o -name "*.h"Length of output: 3528
Core/include/Acts/Detector/ProtoSupport.hpp (1)
16-16: Approve these changes, I do. Hmmmm.Wise changes these are, replacing
BinningDescriptionwithstd::vector<ProtoAxis>. Multiple axes support, it now provides. Clear and purposeful, the transition is.Also applies to: 66-66
Core/include/Acts/Detector/DetectorVolumeBuilder.hpp (1)
15-15: Consistent with the Force, these changes are.Like ripples in the Force, the transition to
ProtoAxisspreads. Maintain the mapping structure while embracing the new way, it does.Also applies to: 46-46
Plugins/DD4hep/src/DD4hepBinningHelpers.cpp (1)
13-13: Verify the constructor parameters, we must.Changed the types have, but unchanged the logic remains. Ensure compatibility with ProtoAxis constructors, we should.
Also applies to: 16-16, 36-36, 52-53, 67-67
✅ Verification successful
Verified the constructor compatibility is, hmm. Strong with the Force, this code is.
In perfect harmony, the constructors are. Match exactly they do:
- Auto-range binning
- Equidistant binning
- Variable binning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ProtoAxis constructor definitions ast-grep --pattern 'ProtoAxis(const AxisDirection&, const AxisBoundaryType&, $$$)'Length of output: 84
Script:
#!/bin/bash # Find ProtoAxis header file fd ProtoAxis.hpp # Search for ProtoAxis class definition ast-grep --pattern 'class ProtoAxis { $$$ }' # Check constructor usage in DD4hepBinningHelpers rg -A 5 'ProtoAxis\(' Plugins/DD4hep/src/DD4hepBinningHelpers.cppLength of output: 764
Script:
#!/bin/bash # View ProtoAxis.hpp content cat Core/include/Acts/Utilities/ProtoAxis.hppLength of output: 6621
Tests/UnitTests/Core/Detector/MultiWireStructureBuilderTests.cpp (1)
75-78: Approve this transition to ProtoAxis, I do.Correctly implemented, the migration from ProtoBinning to ProtoAxis is. Preserved, the functionality remains, while cleaner, the interface becomes.
Tests/UnitTests/Core/Navigation/MultiWireNavigationTests.cpp (1)
79-82: Consistent with the Force, this change is.Mirror the changes in MultiWireStructureBuilderTests.cpp, this implementation does. Harmony in the codebase, it brings.
Core/include/Acts/Material/ProtoSurfaceMaterial.hpp (1)
15-15: Significant architectural improvements, I sense.Three important changes, I observe:
- Replaced ProtoBinning.hpp with ProtoAxis.hpp, we have
- Simplified stream output at line 100, we did
- Updated ProtoGridSurfaceMaterial type to use vector, we have
A more elegant solution, this is. But verify the impact on existing code, we must.
Also applies to: 18-18, 100-100, 114-114
✅ Verification successful
Confirmed, the changes are, hmm. Elegant solutions they bring.
In the file they reside, three changes I see:
- Replaced ProtoBinning.hpp with ProtoAxis.hpp, more modern it is
- Simplified stream output, cleaner code it brings
- Added ProtoGridSurfaceMaterial with vector, flexibility it provides
Safe these changes are, contained within the header they remain. Disturb the Force, they do not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ProtoBinning echo "Searching for remaining ProtoBinning references..." rg "ProtoBinning" --type cpp --type hpp # Search for existing uses of ProtoGridSurfaceMaterial echo "Checking ProtoGridSurfaceMaterial usage..." rg "ProtoGridSurfaceMaterial" --type cpp --type hpp -A 2Length of output: 358
Script:
#!/bin/bash # Search for ProtoBinning references echo "=== Searching for ProtoBinning references ===" rg "ProtoBinning" -t cpp -t h -t hpp -A 2 # Search for ProtoAxis usage echo -e "\n=== Checking ProtoAxis usage ===" rg "ProtoAxis" -t cpp -t h -t hpp -A 2 # Search for ProtoGridSurfaceMaterial echo -e "\n=== Checking ProtoGridSurfaceMaterial usage ===" rg "ProtoGridSurfaceMaterial" -t cpp -t h -t hpp -A 2 # Check stream operator implementations echo -e "\n=== Checking stream operator implementations ===" rg "operator<<.*ProtoSurfaceMaterial" -t cpp -t h -t hpp -A 2Length of output: 716
Script:
#!/bin/bash # Find all header files echo "=== Finding relevant header files ===" fd -e hpp -e h # Search for ProtoGridSurfaceMaterial class definition echo -e "\n=== Checking ProtoGridSurfaceMaterial definition ===" ast-grep --pattern 'class ProtoGridSurfaceMaterial { $$$ }' # Search for include statements echo -e "\n=== Checking includes ===" ast-grep --pattern '#include "Acts/Utilities/ProtoBinning.hpp"' # Search for ProtoAxis usage echo -e "\n=== Checking ProtoAxis usage ===" ast-grep --pattern 'ProtoAxis<$_> $_' # Search for stream operator echo -e "\n=== Checking stream operator ===" ast-grep --pattern 'operator<<($_, const ProtoSurfaceMaterial& $_) { $$$ }'Length of output: 61587
Script:
#!/bin/bash # Check the specific file content echo "=== Checking ProtoSurfaceMaterial.hpp content ===" cat Core/include/Acts/Material/ProtoSurfaceMaterial.hpp # Look for material-related implementations echo -e "\n=== Checking GridSurfaceMaterial usage ===" rg "GridSurfaceMaterial" -t cpp -t h -t hpp -A 3 # Check for vector<ProtoAxis> usage echo -e "\n=== Checking vector<ProtoAxis> usage ===" rg "vector<ProtoAxis" -t cpp -t h -t hpp -A 2Length of output: 4181
Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp (1)
13-13: Clean and focused, this transition is.Three key changes in the Force, I sense:
- Added ProtoAxis.hpp include
- Updated BinningConfig type alias
- Modified handleCylinderBinning signature
But verify the impact on cylinder binning functionality, we must.
Also applies to: 30-30, 91-91
✅ Verification successful
Hmmmm... Safe and well-tested, these changes are.
Through the Force, I sense:
- Comprehensive test coverage for ProtoAxis, I find
- In harmony with the existing codebase, the changes are
- Properly validated in blueprint and integration tests, the functionality remains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for cylinder binning usage patterns echo "Analyzing cylinder binning implementations..." ast-grep --pattern 'handleCylinderBinning($$$)' # Check for any related test cases echo "Checking related test coverage..." rg "handleCylinderBinning" --type cpp -A 5Length of output: 2046
Script:
#!/bin/bash # Check the git history for the file echo "Checking file history..." git show HEAD -- Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp # Look for BinningConfig usage echo "Analyzing BinningConfig usage..." rg "BinningConfig" --type cpp -A 2 # Search for related test files echo "Looking for test files..." fd "MaterialDesignatorBlueprintNode.*test" --type fLength of output: 3719
Script:
#!/bin/bash # Search for integration tests or examples echo "Checking for integration tests..." rg -g "Tests/**" -g "Examples/**" "MaterialDesignatorBlueprintNode" --type cpp -A 3 # Look for ProtoAxis usage in tests echo "Checking ProtoAxis test coverage..." rg -g "Tests/**" "ProtoAxis" --type cpp -A 3Length of output: 22806
Core/include/Acts/Detector/CylindricalContainerBuilder.hpp (1)
17-17: Approve the transition to ProtoAxis, I do. Hmmmm.Correctly implemented, the change to
ProtoAxisis. But verify the compatibility with existing portal material handling, we must.Also applies to: 59-59
✅ Verification successful
Complete and consistent, the transition to ProtoAxis is. Hmmmm.
Successfully verified, the portal material handling has been. No traces of BinningDescription remain, and correctly implemented across core and plugins, the ProtoAxis usage is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining BinningDescription usage rg "BinningDescription" --type cpp # Search for portal material handling code rg "portalMaterialBinning" -A 5 --type cppLength of output: 8907
Core/include/Acts/Detector/LayerStructureBuilder.hpp (1)
19-19: Wise choice, the transition to ProtoAxis is. Hmmmm.Clean and consistent with the project's direction, this change is. Maintain backward compatibility while moving forward, it does.
Also applies to: 85-85
Core/src/Utilities/ProtoAxis.cpp (1)
29-73: Well implemented, the copy operations are, hmm!Proper deep copying of axis and member variables, I see. Careful attention to detail in handling both variable and equidistant axis types, there is.
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Line range hint
34-64: Clear and concise, the documentation is!Well-documented parameters and sensible default values for fillExpansion, I observe. The Force is strong with this one.
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp (1)
65-65: Aligned with the Force, this change is!From ProtoBinning to ProtoAxis, the transition is complete. A clean and focused change, I see.
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
54-66: Strong with testing, this code is. Approve it, I do.Comprehensive tests for copy constructor and assignment operator, you have written. All axis types covered, they are. Balance to the Force, these tests bring.
Also applies to: 143-155, 213-225
Tests/UnitTests/Core/Detector/DetectorVolumeBuilderTests.cpp (1)
163-166: Wise transition to ProtoAxis, this is.Clear and well-structured, the new binning configuration is. Z and Phi axes, properly defined they are. With the Force, this change aligns.
Tests/UnitTests/Plugins/ActSVG/DetectorVolumeSvgConverterTests.cpp (2)
152-154: Hmm, correct the transition to ProtoAxis is, young padawan!For the endcap configuration, maintained the functionality has been, while embracing the new way.
223-227: Wise choice in barrel configuration, I sense!Two axes you have defined, for Z and Phi directions. Clear and well-structured, your code is.
Tests/UnitTests/Core/Detector/LayerStructureBuilderTests.cpp (2)
70-72: Strong with the Force, this test configuration is!Ring structure test, properly migrated to ProtoAxis it has been.
190-194: Well-structured binning configuration, you have created!For the cylinder test, maintained the Z and Phi binning logic has been, while embracing ProtoAxis.
Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp (2)
151-153: JSON conversion tests, properly adapted they are!For endcap configuration, maintained serialization compatibility has been.
231-235: Barrel configuration, properly serialized it shall be!Z and Phi axes, correctly defined they are. JSON conversion tests, complete they remain.
Examples/Python/python/acts/examples/odd_light.py (6)
16-16: Import the Force, you must!Added ProtoAxis to imports, ready for the new way we are.
183-183: Beam pipe binning, simple yet effective it is!Single axis for radial binning, properly converted to ProtoAxis it has been.
194-199: Pixel section binning, well-structured it remains!Both radial and phi binning, maintained their wisdom while embracing ProtoAxis they have.
225-225: PST section binning, clear and concise it is!Single ProtoAxis for radial binning, sufficient for its purpose it remains.
237-242: Short strip section, properly configured it has been!Radial, phi, and Z binning, all converted to ProtoAxis they are. Maintained their structure, they have.
272-277: Long strip section, complete the transition it does!Final section of binning, properly migrated to ProtoAxis it has been. Balance to the Force, this brings!
Core/include/Acts/Utilities/BinUtility.hpp (1)
105-114: Hmmmm, good this constructor looks!Clean and consistent with other constructors, it is. Identity transform and proper initialization, it maintains.
Tests/UnitTests/Core/Detector/CylindricalContainerBuilderTests.cpp (1)
Line range hint
171-174: Wise choice of axis configuration, this is!For cylindrical geometry, proper boundary conditions and binning parameters, you have chosen.
Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp (1)
171-174: Well adapted to new ProtoAxis API, these tests are!Maintained functionality while embracing new interface, you have. Strong with the Force, this code is.
Also applies to: 234-238
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp (1)
492-495: Approve these changes, I do.Successfully transitioned from ProtoBinning to ProtoAxis, you have. Clean and consistent with the PR objectives, your changes are.
Examples/Python/src/Geometry.cpp (1)
Line range hint
389-396: Hmmmm, approve this change, I do!Renamed from
ProtoBinningtoProtoAxis, the class has been. Preserved, the constructor signatures are. Wise decision, this is, aligning with the PR objectives of removingProtoBinningin favor ofProtoAxis, yes.Run this verification script, we must, to ensure no remnants of the old name remain:
✅ Verification successful
Hmmmm, clear of issues, the codebase is!
Variable names
protoBinningsfor vectors ofProtoAxis, acceptable they are. Correctly typed withProtoAxis, all instances are. No action required, there is.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ProtoBinning rg -i "protobinning"Length of output: 775
Examples/Python/python/acts/examples/detector.py (1)
9-9: Hmmmm, good these changes are, yes!Replaced
ProtoBinningwithProtoAxiswisely you have. Maintain the same functionality while embracing the new path, this change does.Also applies to: 27-27
Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp (1)
30-30: Pass the trials, these changes do!Updated the tests to use
ProtoAxisyou have, while maintaining the same test coverage. Wise decision, this is.Also applies to: 263-265
Plugins/GeoModel/src/GeoModelBlueprintCreater.cpp (1)
198-198: In harmony with the Force, these changes are!Transitioned smoothly to
ProtoAxis, the implementation has. Preserved the functionality while embracing the new way, you did.Also applies to: 387-388
Core/include/Acts/Utilities/BinningData.hpp (1)
15-15: Approve include statement addition.Hmmmm, properly placed, the include statement is. Clear dependency on ProtoAxis, we have.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
Core/include/Acts/Detector/detail/IndexedSurfacesGenerator.hpp (1)
72-127: A single templated approach for 1D and 2D, consider you might.Very concise, your 2D factory method is. Mirror the logic, it does, from 1D. If code duplication too high becomes, unify with a template parameter for dimensions, you could.
Tests/UnitTests/Core/Detector/IndexedSurfacesGeneratorTests.cpp (3)
55-65: Thorough coverage for RingDisc1D, you bring.Yes, tested the radial surfaces are; 44 φ-bins used, correct it seems. Advised it is to add boundary edge checks, verifying no bin overlaps at ±π, you might.
156-167: RingDisc2D test, clarity it shows.Testing two rings in both r and φ, well done. Overlap conditions test thoroughly, you should, especially near axis boundaries.
261-270: Expanded bin coverage scenario, interesting it is.Confidence in fill expansions, the test exudes. Good practice it would be to measure performance in large expansions, yes.
Tests/UnitTests/Plugins/ActSVG/IndexedSurfacesSvgConverterTests.cpp (1)
168-177: Consider the performance impact of fine binning, you should!Increased bin count, performance implications it may have. For large datasets, consider coarser binning or dynamic adjustment.
Core/include/Acts/Detector/detail/ProtoMaterialHelper.hpp (1)
20-28: Update the documentation comments, you must.Outdated, these comments are. Speak of BinningDescription they do, when vector of ProtoAxis we now use.
Apply these changes to the documentation, you should:
/// @brief Method that attaches proto material to a surface given -/// a proto binning description +/// a vector of proto axes /// /// @param gctx is the geometry context, needed for extent measuring /// @param surface is the portal where the material is attached -/// @param bDescription is the binning description for the proto binning +/// @param bDescription is the vector of proto axes for material binning /// -/// @return an (eventual) updated binning description for structured +/// @return an (eventual) updated vector of proto axes for structured /// screen logging outputTests/UnitTests/Core/Geometry/BlueprintTests.cpp (1)
518-521: Updated access methods, consistent they are!The new access pattern through
getAxis().getNBins()follows the ProtoAxis API correctly. Matches the binning values set in the test configuration above, it does.Yet, test more edge cases, we should:
Consider adding test cases for:
+ // Test invalid axis configurations + BOOST_CHECK_THROW(ProtoAxis{AxisR, Bound, 0}, std::invalid_argument); + + // Test axis range validation + BOOST_CHECK_THROW(ProtoAxis{AxisR, Bound, -1}, std::invalid_argument);Core/src/Detector/MultiWireStructureBuilder.cpp (1)
82-92: Refactor opportunity, I sense.Extract axis creation into a helper function, we should. Improve readability and reusability, it will.
+namespace { +template <typename T> +Acts::Axis<Acts::AxisType::Equidistant, Acts::AxisBoundaryType::Bound> +createEquidistantAxis(const T& iaxis) { + return Acts::Axis<Acts::AxisType::Equidistant, Acts::AxisBoundaryType::Bound>( + iaxis.getBinEdges().front(), iaxis.getBinEdges().back(), + iaxis.getNBins()); +} +} // namespace - Acts::Axis<Acts::AxisType::Equidistant, Acts::AxisBoundaryType::Bound> - axisA(iaxisA.getBinEdges().front(), iaxisA.getBinEdges().back(), - iaxisA.getNBins()); - - Acts::Axis<Acts::AxisType::Equidistant, Acts::AxisBoundaryType::Bound> - axisB(iaxisB.getBinEdges().front(), iaxisB.getBinEdges().back(), - iaxisB.getNBins()); + auto axisA = createEquidistantAxis(iaxisA); + auto axisB = createEquidistantAxis(iaxisB);Core/src/Detector/LayerStructureBuilder.cpp (1)
200-232: Improved error handling and navigation setup, I observe.Clear separation between 1D and 2D binning cases, you have achieved. But a magic number, I spot.
+ // Maximum supported binning dimensions + constexpr std::size_t kMaxBinningDimensions = 2u; + if (binnings.empty()) { ACTS_DEBUG( "No surface binning provided, navigation will be 'tryAll' " "(potentially slow)."); } else { // Sort the binning for conventions std::ranges::sort(binnings, {}, [](const auto& b) { return b.getAxisDirection(); }); + if (binnings.size() > kMaxBinningDimensions) { + throw std::runtime_error( + "LayerStructureBuilder: only 1D or 2D surface binning " + "supported."); + }Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
135-148: Expand test coverage, young padawan must.Good test cases you have written, but more comprehensive they must become. Consider these scenarios you should:
- Error handling for invalid axis parameters
- Edge cases for bin boundaries
- Verification of transformed coordinates
BOOST_AUTO_TEST_CASE(BinUtility_from_ProtoAxis) { using enum AxisDirection; using enum AxisBoundaryType; + // Test invalid parameters + BOOST_CHECK_THROW({ + ProtoAxis invalid(AxisX, Bound, 1.0, 0.0, 10); // max < min + BinUtility bu(invalid); + }, std::invalid_argument); + ProtoAxis epabX(AxisX, Bound, 0.0, 1.0, 10); BinUtility buX(epabX); BOOST_CHECK_EQUAL(buX.bins(), std::size_t{10}); BOOST_CHECK_EQUAL(buX.dimensions(), std::size_t{1}); + // Test edge cases + Vector3 edgePos(1.0, 0.0, 0.0); // At boundary + BOOST_CHECK_EQUAL(buX.bin(edgePos), std::size_t{9}); + ProtoAxis epabY(AxisY, Bound, 0.0, 1.0, 10); BinUtility buXY({epabX, epabY}); BOOST_CHECK_EQUAL(buXY.bins(), std::size_t{100}); BOOST_CHECK_EQUAL(buXY.dimensions(), std::size_t{2}); + + // Test with transformation + Transform3 rot = Transform3::Identity(); + rot.rotate(AngleAxis3(M_PI/4, Vector3::UnitZ())); + BinUtility buXYrot(buXY); + buXYrot.transform() = rot; + Vector3 pos(0.5, 0.0, 0.0); + BOOST_CHECK_NE(buXY.bin(pos), buXYrot.bin(pos)); }Core/src/Utilities/ProtoAxis.cpp (3)
28-44: Constructors well written they are, but improvements possible they remain.Handle memory management with smart pointers we should. Consider std::unique_ptr for m_axis:
-IAxis* m_axis; +std::unique_ptr<IAxis> m_axis;
104-130: Improve edge case handling and performance, we must.In variable axis case, vector operations optimized they should be. Reserve space before modifications, we must:
} else { std::vector<double> edges = m_axis->getBinEdges(); + edges.reserve(edges.size() + 2); // Reserve space for minE and maxE // Clip it to min/max edges.erase(std::remove_if(edges.begin(), edges.end(), [minE, maxE](double e) -> bool { return (e < minE || e > maxE); }), edges.end());
161-173: Optimize stream operations, we shall.For vector streaming, avoid repeated string conversions we must:
std::ostream& Acts::operator<<(std::ostream& os, const std::vector<ProtoAxis>& as) { + if (as.empty()) { + return os; + } for (const auto& a : as) { - os << a.toString() << '\n'; + os << a; + if (&a != &as.back()) { + os << '\n'; + } } return os; }Core/include/Acts/Utilities/BinUtility.hpp (1)
108-127: Optimize constructors with move semantics, we must.More efficient the vector constructor can be:
- BinUtility(const std::vector<ProtoAxis>& pAxes) + BinUtility(std::vector<ProtoAxis> pAxes) : m_binningData(), m_transform(Transform3::Identity()), m_itransform(Transform3::Identity()) { m_binningData.reserve(3); - for (const auto& pAxis : pAxes) { - m_binningData.push_back(BinningData(pAxis)); + m_binningData.reserve(pAxes.size()); + for (auto&& pAxis : pAxes) { + m_binningData.emplace_back(std::move(pAxis)); } }Tests/UnitTests/Core/Utilities/BinningDataTests.cpp (1)
428-458: More test cases add, we must.Good foundation laid you have, but more thorough testing needed it is:
BOOST_AUTO_TEST_CASE(BinningData_from_ProtoAxis) { using enum AxisDirection; using enum AxisBoundaryType; + // Test closed boundary type with phi axis + ProtoAxis phiAxis(AxisPhi, Closed, -M_PI, M_PI, 10); + BinningData bPhi(phiAxis); + BOOST_CHECK(bPhi.option == closed); + + // Test invalid boundary type + BOOST_CHECK_THROW({ + ProtoAxis invalid(AxisX, Closed, 0.0, 1.0, 10); + }, std::invalid_argument); + // Bound, equidistant axis ProtoAxis epab(AxisX, Bound, 0.0, 1.0, 10); BinningData bEpab(epab);Core/include/Acts/Utilities/BinningData.hpp (1)
178-201: Good implementation, but type safety improve we must.Explicit type conversions from double to float, potential precision loss they bring. Consider these improvements:
- Static assertions or runtime checks for value ranges, add we should.
- Document potential precision loss in comments, we must.
Apply this change, you should:
explicit BinningData(const ProtoAxis& pAxis) : binvalue(pAxis.getAxisDirection()), subBinningData(nullptr) { const auto& axis = pAxis.getAxis(); type = axis.getType() == AxisType::Equidistant ? equidistant : arbitrary; option = axis.getBoundaryType() == AxisBoundaryType::Closed ? closed : open; + // Note: Potential precision loss from double to float conversion + static_assert(std::numeric_limits<float>::max() >= 1e6, + "Float range insufficient for typical axis values"); min = static_cast<float>(axis.getMin()); max = static_cast<float>(axis.getMax()); m_bins = axis.getNBins(); step = (max - min) / (1. * m_bins); zdim = (m_bins == 1); m_boundaries.reserve(axis.getBinEdges().size()); for (const auto& edge : axis.getBinEdges()) { m_boundaries.push_back(static_cast<float>(edge)); } m_totalBins = m_bins; m_totalBoundaries = m_boundaries; // Set the search function pointer based on axis type m_functionPtr = (type == equidistant) ? &searchEquidistantWithBoundary : &searchInVectorWithBoundary; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Core/include/Acts/Detector/detail/IndexedSurfacesGenerator.hpp(1 hunks)Core/include/Acts/Detector/detail/ProtoMaterialHelper.hpp(2 hunks)Core/include/Acts/Utilities/BinUtility.hpp(2 hunks)Core/include/Acts/Utilities/BinningData.hpp(2 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(6 hunks)Core/src/Detector/LayerStructureBuilder.cpp(3 hunks)Core/src/Detector/MultiWireStructureBuilder.cpp(3 hunks)Core/src/Utilities/ProtoAxis.cpp(3 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Plugins/ActSVG/include/Acts/Plugins/ActSVG/IndexedSurfacesSvgConverter.hpp(0 hunks)Plugins/DD4hep/src/DD4hepBinningHelpers.cpp(4 hunks)Plugins/Json/include/Acts/Plugins/Json/IndexedSurfacesJsonConverter.hpp(0 hunks)Tests/UnitTests/Core/Detector/IndexedSurfacesGeneratorTests.cpp(7 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(2 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(3 hunks)Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp(2 hunks)Tests/UnitTests/Core/Utilities/BinningDataTests.cpp(2 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(3 hunks)Tests/UnitTests/Plugins/ActSVG/IndexedSurfacesSvgConverterTests.cpp(6 hunks)
💤 Files with no reviewable changes (2)
- Plugins/Json/include/Acts/Plugins/Json/IndexedSurfacesJsonConverter.hpp
- Plugins/ActSVG/include/Acts/Plugins/ActSVG/IndexedSurfacesSvgConverter.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- Examples/Python/src/Geometry.cpp
- Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp
- Plugins/DD4hep/src/DD4hepBinningHelpers.cpp
- Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
- Core/include/Acts/Utilities/ProtoAxis.hpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/Utilities/BinningData.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (22)
Core/include/Acts/Detector/detail/IndexedSurfacesGenerator.hpp (2)
20-71: Precise your axis validations, you should.Happy I am with the structure for creating a 1D indexed grid. Yet handle scenarios with empty surfaces or invalid axes, you could. Edge cases properly validated, they must be, or out-of-bounds errors you may face.
129-129: Gratitude for the new namespace boundary, I express.Makes the code organization consistent, does it, yes.
Tests/UnitTests/Core/Detector/IndexedSurfacesGeneratorTests.cpp (4)
28-28: Include welcomed, it is.Good that
ProtoAxis.hppis included, the new axis code it leverages, hmm.
100-114: Extended test with additional disk, promising it is.Ensure you confirm correct indexing of the last index, do you. Surfaces appended at the end, off-by-one issues avoid, must you.
208-218: Finer radial bins, covered they are.In-depth coverage for ring discs with narrower bin sizes you have. Further expansions tested, you might, to ensure no performance issues.
300-310: Cylinder2D test, robust it seems.In Z and φ, your axis coverage extends. Overlapping bins or dead zones, watch for, you must. Beyond that, strong coverage, you have.
Tests/UnitTests/Plugins/ActSVG/IndexedSurfacesSvgConverterTests.cpp (5)
80-85: Approve the transition to ProtoAxis, I do!Wisely implemented, the change from ProtoBinning to ProtoAxis is. Clear and concise, the new implementation remains.
132-142: Well structured, the 2D navigation implementation is!Correctly defined, both R and phi axes are. Proper boundary types and values, they have.
205-208: Wise choice of expansion factors, verify you must!Added expansion factors (2u for R, 4u for phi), impact on search efficiency they have. Balance between search window size and lookup performance, maintain you should.
228-237: Consistent with other test cases, this implementation is!Well chosen, the Z and phi axes parameters are. Minimal expansion factors, wise choice they are.
99-99: Verify the impact of RadialBounds change, we must!Changed from 20 to 10, the lower bound has been. Impact on test behavior, this may have.
✅ Verification successful
Bounds change impact verified, it is.
Tests using RadialBounds now consistently employ a lower bound of 10 (e.g., in both IndexedSurfacesSvgConverterTests.cpp and IndexedSurfacesGeneratorTests.cpp). No additional discrepancies nor unexpected behaviors are apparent in relation to this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tests using RadialBounds to ensure consistency rg "RadialBounds.*10\." -A 2Length of output: 1535
Core/include/Acts/Detector/detail/ProtoMaterialHelper.hpp (3)
12-12: Hmmmm, wise addition of ProtoAxis include, this is.Necessary for the new types, this header inclusion is. Aligned with the migration from ProtoBinning to ProtoAxis, it stands.
18-18: Structured namespace organization, I sense.Clear and organized, the namespace hierarchy becomes. Experimental::detail, a good home for implementation details, it is.
29-31: Changed the function signature has. Verify its impact, we must.Replaced BinningDescription with std::vector, the Force guides us. But careful we must be, for changes ripple through the codebase they may.
✅ Verification successful
Hmm, additional verification we must perform. Run the above script, you should, to check thoroughly that all remnants of BinningDescription are gone and that every attachProtoMaterial call is truly updated. The Force, though it seems to reveal no issues at first glance, cautious confirmation demands. Once new output we see, further judgment we can deliver.
Function signature update verified, it has.
The search reveals no remaining references to BinningDescription, and all calls to attachProtoMaterial appear updated to use the new signature with std::vector<ProtoAxis>. The changes ripple through the codebase correctly, they do.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of BinningDescription that might need updating rg "BinningDescription" --type cpp # Find all calls to attachProtoMaterial to ensure they're updated ast-grep --pattern 'attachProtoMaterial($$$)'Length of output: 82
Script:
#!/bin/bash echo "Searching repository for any reference to 'BinningDescription':" rg "BinningDescription" -n . echo "Searching repository for function calls to 'attachProtoMaterial(' (a likely function call):" rg "attachProtoMaterial\(" -n .Length of output: 1120
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp (2)
32-32: Hmm, include this header, you must!Correctly added, the ProtoAxis header is. Aligns with the PR's objective to replace ProtoBinning, it does.
493-496: Elegant and clear, this transition to ProtoAxis is!The migration from ProtoBinning to ProtoAxis, well-structured it is. For both NegativeDisc and PositiveDisc, proper axis configuration maintained, it has been.
Verify the axis configuration across the codebase, we should:
✅ Verification successful
Hmm, a further quest for verification we must undertake. The initial ast-grep command yielded no output, uncertain its reach was. Now, with this new search using
rg, additional instances of the ProtoAxis configuration across the codebase shall be uncovered. Once the script runs and its output be provided, clearer our understanding will become.May the force of rigorous review be with you.
Axis configuration verified, it is!
Across the codebase, instances of
ProtoAxisare present in multiple test files and implementation headers. In BlueprintTests.cpp, the use ofProtoAxis{AxisR, Bound, ...}andProtoAxis{AxisPhi, Bound, ...}is consistent with the expected configuration forNegativeDiscandPositiveDisc, while other tests (e.g., in JSON and Detector tests) utilizeProtoAxiswithAxisZappropriately. No discrepancies or misconfigurations observed, they have been.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of ProtoAxis configuration ast-grep --pattern 'ProtoAxis{$_, $_, $_}'Length of output: 44
Script:
#!/bin/bash # Searching for all occurrences of 'ProtoAxis' configuration across the repository to verify proper axis setup. rg "ProtoAxis\s*\{" .Length of output: 1296
Core/src/Detector/MultiWireStructureBuilder.cpp (3)
40-40: Approve the transition to ProtoAxis, I do.A wise change this is, aligning with the new architecture.
94-123: Clear and concise, the navigation setup is.Well-structured code for setting up grid navigation and delegates, you have written.
68-80: Strong with validation, this code is.Proper checks for binning axes count and type, you have added. Prevent runtime errors, this will.
✅ Verification successful
Master Yoda speaks: "To the force of validation, our gaze we now turn. Search we must for consistency in binning checks in MultiWireStructureBuilder.cpp. When these results appear, our understanding shall be complete."
Attention, young padawan: Bin validations verified, they are.
The search shows that only the expected binning count validation exists (as seen on line 68) and no additional or inconsistent equidistant-check code is found. The existing checks in the code assure that at least two binning axes are present and that both axes are equidistant. This is consistent and correct, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of ProtoAxis to ensure consistent validation ast-grep --pattern 'if ($axis.getType() != Acts::AxisType::Equidistant) { $$$ }'Length of output: 84
Script:
#!/bin/bash echo "Searching for equidistant binning validations in MultiWireStructureBuilder.cpp" rg 'iaxisA.getType() != Acts::AxisType::Equidistant' -n "Core/src/Detector/MultiWireStructureBuilder.cpp" rg 'iaxisB.getType() != Acts::AxisType::Equidistant' -n "Core/src/Detector/MultiWireStructureBuilder.cpp" echo "Searching for binning count validation in MultiWireStructureBuilder.cpp" rg 'm_cfg\.binning\.size\(\)' -n "Core/src/Detector/MultiWireStructureBuilder.cpp"Length of output: 775
Core/src/Detector/LayerStructureBuilder.cpp (1)
46-62: Simpler and cleaner, the binning range adaptation has become.Good refactoring, this is. More maintainable, the code now is.
Core/include/Acts/Utilities/BinningData.hpp (2)
15-15: Approve the include statement, I do.Necessary for the new constructor from ProtoAxis, this include is.
199-200: Function pointer initialization, approve I do.Correctly addressed the past review comment about function pointer initialization, this implementation has.
This PR removes the
ProtoBinningand changes it to the newProtoAxis. For a bit of time, it will be possible to create aBinUtilityfrom one or moreProtoAxis, but this will disappear with the phase out ofBinUtilityandBinnedArray.To do still on this PR:
ProtoAxis(mainly copying and assignment)BinUtility(albeit will vanish)The compilation of the
LayerStructuregot heavier as expected. As discussed, this could/should be tackled in the future with abstracting the function body away that does the filling, etc.--- END COMMIT MESSAGE ---
@paulgessinger @ssdetlab - to make you aware
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Here are the release notes for this pull request:
New Features
ProtoAxisas a more flexible and robust replacement forProtoBinning.Breaking Changes
ProtoBinningandBinningDescriptionstructures.ProtoAxisinstead ofProtoBinning.Improvements
ProtoAxis.Refactoring