chore: Cleanup some surface code #3991
Conversation
WalkthroughHmm, a comprehensive refactoring of surface-related classes in the ACTS project, this is! Namespace prefixes removed, method signatures streamlined, and consistency checks added across multiple surface and bounds implementations. A systematic cleanup of code infrastructure, we witness. Inline method definitions, new utility methods like Changes
Sequence DiagramsequenceDiagram
participant Developer
participant SurfaceClass
participant BoundsClass
Developer->>SurfaceClass: Refactor Methods
SurfaceClass->>BoundsClass: Update Signatures
BoundsClass-->>SurfaceClass: Return Consistent Interface
Developer->>SurfaceClass: Add Utility Methods
SurfaceClass-->>Developer: Improved Code Structure
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (6)
Core/src/Surfaces/DiscTrapezoidBounds.cpp (1)
Line range hint
75-85: Duplicate code, I sense in the Force!Calculate vertices twice, we should not. Use the vertices() method, we must.
- Vector2 vertices[] = {{get(eHalfLengthXminR), get(eMinR)}, - {get(eHalfLengthXmaxR), m_ymax}, - {-get(eHalfLengthXmaxR), m_ymax}, - {-get(eHalfLengthXminR), get(eMinR)}}; + auto vertexList = vertices(0); + Vector2 vertices[4] = {vertexList[0], vertexList[1], vertexList[2], vertexList[3]};Core/src/Surfaces/CylinderBounds.cpp (1)
Line range hint
55-102: Break down this function into smaller parts, we must.Complex, this function has become. Readability suffer, it does. Extract helper methods for bevel calculations and polygon checks, I suggest.
Consider extracting these helper methods:
bool checkStandardBounds(const Vector2& shiftedPosition, double halfPhi, double halfLengthZ) const; bool checkBeveledBounds(const Vector2& position, double radius, double halfLengthZ) const; std::array<Vector2, 6> createBevelVertices(double radius, double halfLengthZ) const;Plugins/Json/src/SurfaceJsonConverter.cpp (1)
Line range hint
159-185: Named constants for type values, use you must.Magic numbers in type assignment, clarity they lack. Enum or constants, better understanding they bring.
+ enum class DetrayType { + Portal = 0, + Sensitive = 1, + Passive = 2 + }; + - jSurface["type"] = options.portal ? 0 : (surface.geometryId().sensitive() > 0 ? 1u : 2u); + jSurface["type"] = options.portal ? + static_cast<unsigned>(DetrayType::Portal) : + (surface.geometryId().sensitive() > 0 ? + static_cast<unsigned>(DetrayType::Sensitive) : + static_cast<unsigned>(DetrayType::Passive));Core/src/Surfaces/Surface.cpp (1)
Line range hint
281-324: A disturbance in the Force, I sense! Solution2 calculation incorrect, it is!- Vector3 solution2 = position + qe.first * direction; + Vector3 solution2 = position + qe.second * direction;Using first solution instead of second, the code is. Fixed this must be, or incorrect intersections you will have!
Core/src/Surfaces/ConeSurface.cpp (1)
Line range hint
258-280: Magic numbers in your code, I see. Constants define, you should!+ constexpr double EPSILON = 1e-16; if (A == 0.) { - A += 1e-16; // avoid division by zero + A += EPSILON; // avoid division by zero }More maintainable and clear, your code will be.
Core/src/Surfaces/AnnulusBounds.cpp (1)
Line range hint
196-362: Cache calculations for better performance, we should!Many repeated calculations, I see. Extract common subexpressions:
+ const double weightedNorm = (n.transpose() * weight * n).value(); + const double weightedDiff = ((p - a).transpose() * weight * n).value(); - auto f = (n.transpose() * weight * n).value(); - auto u = ((p - a).transpose() * weight * n).value() / f; + auto u = weightedDiff / weightedNorm;Less computation, more speed, this brings!
🧹 Nitpick comments (57)
Core/src/Surfaces/detail/VerticesHelper.cpp (3)
Line range hint
72-115: More descriptive names for variables, suggest I do.Cryptic they are, these variable names:
rverticescould beresultVerticesiverticescould beinnerVerticesoverticescould beouterVerticesEasier to read, the code would be, hmm?
116-122: Documentation, this function needs.A brief comment explaining that this is a specialized case of ellipsoid vertices with equal radii, helpful would be. Clear intentions make, we must!
+// Specialized case of ellipsoid vertices where inner and outer radii are circular (equal in x and y) std::vector<Vector2> detail::VerticesHelper::circularVertices( double innerR, double outerR, double avgPhi, double halfPhi, unsigned int quarterSegments) {
Line range hint
123-139: Default tolerance value, consider adding you should.A reasonable default value for the tolerance parameter, beneficial it would be. Less burden on the caller, this brings.
-bool detail::VerticesHelper::onHyperPlane(const std::vector<Vector3>& vertices, - double tolerance) { +bool detail::VerticesHelper::onHyperPlane(const std::vector<Vector3>& vertices, + double tolerance = 1e-6) {Core/src/Surfaces/RectangleBounds.cpp (3)
19-33: Clear and precise, this implementation is!Handle all boundary values with grace, it does. Yet, a small enhancement suggest, I must.
Add this documentation, you should:
+/// @brief Get the boundary value for the specified bound type +/// @param bValue The boundary value type to retrieve +/// @return The requested boundary value, or NaN if invalid double RectangleBounds::get(BoundValues bValue) const {
35-42: Strong with the Force, this validation is!Yet, optimize it further, we can. A single check, more elegant would be.
Apply this optimization, you might:
void RectangleBounds::checkConsistency() noexcept(false) { - if (get(eMinX) > get(eMaxX)) { - throw std::invalid_argument("RectangleBounds: invalid local x setup"); - } - if (get(eMinY) > get(eMaxY)) { - throw std::invalid_argument("RectangleBounds: invalid local y setup"); - } + const bool validX = get(eMinX) <= get(eMaxX); + const bool validY = get(eMinY) <= get(eMaxY); + if (!validX || !validY) { + throw std::invalid_argument("RectangleBounds: invalid " + + std::string(!validX ? "x" : "y") + " setup"); + } }
Line range hint
60-70: Clear output formatting, this method provides!Yet, modernize it further, we can. The way of std::format, more elegant it is.
When C++20 available becomes, transform it thus:
- sl << "Acts::RectangleBounds: (hlX, hlY) = " - << "(" << 0.5 * (get(eMaxX) - get(eMinX)) << ", " - << 0.5 * (get(eMaxY) - get(eMinY)) << ")"; + sl << std::format("Acts::RectangleBounds: (hlX, hlY) = ({}, {})", + 0.5 * (get(eMaxX) - get(eMinX)), + 0.5 * (get(eMaxY) - get(eMinY)));Core/src/Surfaces/RadialBounds.cpp (1)
33-43: Strong with the Force, this validation is!Robust error checking, you have implemented. But split the validation logic into smaller functions, we should, for better readability and maintenance.
Consider this refactoring, you might:
+bool RadialBounds::hasValidRadii() const { + return !(get(eMinR) < 0. || get(eMaxR) <= 0. || get(eMinR) > get(eMaxR)); +} + +bool RadialBounds::hasValidPhiSector() const { + return !(get(eHalfPhiSector) < 0. || get(eHalfPhiSector) > std::numbers::pi); +} + void RadialBounds::checkConsistency() noexcept(false) { - if (get(eMinR) < 0. || get(eMaxR) <= 0. || get(eMinR) > get(eMaxR)) { + if (!hasValidRadii()) { throw std::invalid_argument("RadialBounds: invalid radial setup"); } - if (get(eHalfPhiSector) < 0. || get(eHalfPhiSector) > std::numbers::pi) { + if (!hasValidPhiSector()) { throw std::invalid_argument("RadialBounds: invalid phi sector setup."); } if (get(eAveragePhi) != detail::radian_sym(get(eAveragePhi))) { throw std::invalid_argument("RadialBounds: invalid phi positioning."); } }Core/src/Surfaces/TrapezoidBounds.cpp (5)
Line range hint
26-45: Hmm, strong with the Force these constructors are!Proper validation and initialization, I sense. Yet, a small suggestion I have - document the valid ranges of parameters in the constructor comments, we should. Help future Padawans, it will.
Add parameter constraints to documentation, like this you should:
/// Constructor for symmetric Trapezoid /// /// @param halfXnegY minimal half length X, definition at negative Y +/// @note halfXnegY must be positive /// @param halfXposY maximal half length X, definition at positive Y +/// @note halfXposY must be positive /// @param halfY half length Y - defined at x=0 +/// @note halfY must be positive /// @param rotAngle: rotation angle of the bounds w.r.t coordinate axes
Line range hint
51-93: Wise implementation of boundary check, this is!Fast path for simple cases, slow path for complex ones - balanced like the Force, it is. Yet, consider we should, caching the rotation matrix when angle remains constant.
+ // Cache these values if they're frequently used + const auto rotMatrix = Eigen::Rotation2Dd(rotAngle); - const Vector2 extPosition = Eigen::Rotation2Dd(rotAngle) * lposition; + const Vector2 extPosition = rotMatrix * lposition;
95-108: Clear as Kamino's waters, this vertices calculation is!Yet, mysterious the ignored parameter remains. Document its purpose, we must, or remove if unnecessary, we should.
-std::vector<Vector2> TrapezoidBounds::vertices( - unsigned int /*ignoredSegments*/) const { +/// @param segments Ignored for trapezoid as vertices are always 4 +std::vector<Vector2> TrapezoidBounds::vertices( + unsigned int segments [[maybe_unused]]) const {
124-128: Efficient like a Padawan's first lightsaber, this method could be!Reserve the vector's capacity before insertion, we should. Prevent unnecessary allocations, it will.
std::vector<double> TrapezoidBounds::values() const { std::vector<double> valvector; + valvector.reserve(m_values.size()); valvector.insert(valvector.begin(), m_values.begin(), m_values.end()); return valvector; }
138-145: Strong validation I sense, but stronger it could become!Additional checks for rotation angle bounds and relative lengths, consider we should.
void TrapezoidBounds::checkConsistency() noexcept(false) { if (get(eHalfLengthXnegY) <= 0. || get(eHalfLengthXposY) <= 0.) { throw std::invalid_argument("TrapezoidBounds: invalid local x setup"); } if (get(eHalfLengthY) <= 0.) { throw std::invalid_argument("TrapezoidBounds: invalid local y setup"); } + // Validate rotation angle is within [-π, π] + if (std::abs(get(eRotationAngle)) > M_PI) { + throw std::invalid_argument("TrapezoidBounds: rotation angle must be within [-π, π]"); + } }Core/src/Surfaces/DiscTrapezoidBounds.cpp (6)
23-31: Wise defensive programming, I sense in this constructor!Validate parameters before calculations, it does. Yet, document the geometric constraints in comments, we should.
Consider adding this documentation:
// Constructor for DiscTrapezoidBounds // @param halfXminR Half-length in x at minimum radius // @param halfXmaxR Half-length in x at maximum radius // @param minR Minimum radius of the trapezoid // @param maxR Maximum radius of the trapezoid // @param avgPhi Average phi angle position // @param stereo Stereo angle // @throws std::invalid_argument if geometric constraints are violated
37-41: Optimize the values() method, we must!Unnecessary temporary vector and copy operations, I sense. Return a reference to const internal values, we should.
-std::vector<double> DiscTrapezoidBounds::values() const { - std::vector<double> valvector; - valvector.insert(valvector.begin(), m_values.begin(), m_values.end()); - return valvector; -} +const std::array<double, 6>& DiscTrapezoidBounds::values() const { + return m_values; +}
43-54: More specific in our error messages, we must be!Good validation logic you have, but guide the users better with detailed error messages, we should.
- throw std::invalid_argument("DiscTrapezoidBounds: invalid radial setup."); + throw std::invalid_argument("DiscTrapezoidBounds: Invalid radial setup. " + "Requires 0 <= minR <= maxR, got minR=" + + std::to_string(get(eMinR)) + + ", maxR=" + std::to_string(get(eMaxR)));
Line range hint
56-73: Document the mathematical transformations, we should!Correct the calculations are, but mysterious to young padawans they might seem. Add mathematical documentation, we must.
Consider adding this documentation:
// Transforms from polar (r,phi) to local Cartesian coordinates (x,y) // x = r * sin(phi - avgPhi) // y = r * cos(phi - avgPhi)
Line range hint
87-98: Document the ignored parameter, we must!Mysterious, the ignored parameter is. Explain its purpose in documentation, we should.
Add parameter documentation:
/// @param ignoredSegments Parameter kept for interface consistency with other /// bounds types but not used in this implementation
Line range hint
101-115: Const correctness in string literals, maintain we should!Good output formatting you have, but const string literals, safer they are.
- sl << "Acts::DiscTrapezoidBounds: (innerRadius, outerRadius, " + sl << std::string_view{"Acts::DiscTrapezoidBounds: (innerRadius, outerRadius, " "halfLengthXminR, " "halfLengthXmaxR, halfLengthY, halfPhiSector, averagePhi, rCenter, " - "stereo) = "; + "stereo) = "};Core/include/Acts/Surfaces/TrapezoidBounds.hpp (2)
Line range hint
89-89: Optimize for speed, we could. A TODO comment, lingering it is.Cache the kappa/delta calculations, the comment suggests. Performance benefits, this might bring.
Help implement this optimization, shall I? Create a GitHub issue to track this enhancement, we could.
Line range hint
146-150: Consistent in our ways with exceptions, we must be.Missing noexcept specifications on some methods, I see. Add them we should, where appropriate they are.
- std::ostream& toStream(std::ostream& sl) const final; + std::ostream& toStream(std::ostream& sl) const noexcept(false) final; - double get(BoundValues bValue) const { return m_values[bValue]; } + double get(BoundValues bValue) const noexcept { return m_values[bValue]; }Core/include/Acts/Surfaces/RectangleBounds.hpp (1)
104-115: Documentation improvements, suggest I do!Clear the code is, yet stronger with proper documentation it becomes. Add we should:
- For halfLengthX/Y: Document the return value represents the distance from center to edge
- For min/max: Clarify the coordinate system and units used
A Jedi's strength flows from knowledge well-documented!
Apply these changes, you should:
/// Access to the half length in X + /// @return Half the length in X direction, measured from center to edge double halfLengthX() const { return 0.5 * (m_max.x() - m_min.x()); } /// Access to the half length in Y + /// @return Half the length in Y direction, measured from center to edge double halfLengthY() const { return 0.5 * (m_max.y() - m_min.y()); } /// Get the min vertex defining the bounds + /// @return The minimum corner coordinates (x,y) in the local frame const Vector2& min() const { return m_min; } /// Get the max vertex defining the bounds + /// @return The maximum corner coordinates (x,y) in the local frame const Vector2& max() const { return m_max; }Core/src/Surfaces/detail/MergeHelper.cpp (1)
Line range hint
89-90: More descriptive error messages, suggest I do!When incompatible phi ranges we encounter, more details in the error message, helpful they would be. The actual values that caused the incompatibility, include we should.
Apply this improvement, you might:
- ACTS_ERROR("Phi ranges are incompatible"); - throw std::invalid_argument("Phi ranges are incompatible"); + ACTS_ERROR("Phi ranges are incompatible: range1=[" << minPhi1 / 1_degree + << ", " << maxPhi1 / 1_degree << "], range2=[" + << minPhi2 / 1_degree << ", " << maxPhi2 / 1_degree << "]"); + throw std::invalid_argument("Phi ranges are incompatible: ranges do not connect at any point");Core/include/Acts/Surfaces/InfiniteBounds.hpp (1)
20-20: Enhance the class documentation, young padawan must.Incomplete, the class documentation is. Add information about:
- Purpose of the infinite bounds
- Use cases where this class applies
- Example usage scenarios
Core/src/Surfaces/LineBounds.cpp (1)
23-24: Consistent spacing in parameter list, maintain we must.Uneven indentation in the parameter list, I sense. Align with the Force, we shall:
-bool LineBounds::inside(const Vector2& lposition, - const BoundaryTolerance& boundaryTolerance) const { +bool LineBounds::inside(const Vector2& lposition, + const BoundaryTolerance& boundaryTolerance) const {Core/src/Surfaces/ConvexPolygonBounds.cpp (2)
70-73: Document the exceptions, we should.Though noexcept(false) specified correctly is, document the specific exceptions thrown, we must. Help future Jedi understand error conditions, it will.
Line range hint
18-75: Strong with the Force, these namespace changes are!Clean and consistent, the code has become. Proper encapsulation maintained while reducing verbosity. Approve these changes, I do!
A suggestion for future improvements, I have:
- Consider adding static_assert for minimum vertex count
- Document the geometric constraints for valid polygons
Core/src/Surfaces/PerigeeSurface.cpp (1)
Line range hint
61-80: Complex geometry calculation, careful review it needs!In polyhedronRepresentation, fixed values for wire length (-100, 100) hardcoded they are. More flexible parameters, consider we should.
Consider parameterizing the wire length:
- Vector3 left(0, 0, -100.); - Vector3 right(0, 0, 100.); + const double wireLength = 100.; // Consider making this configurable + Vector3 left(0, 0, -wireLength); + Vector3 right(0, 0, wireLength);Core/include/Acts/Surfaces/SurfaceBounds.hpp (1)
Line range hint
58-61: Pure virtual interface, strong foundation it provides!Values method, dynamic vector it returns. Consider adding size hints or constraints, helpful for derived classes they would be.
/// @return vector of parameters (size depends on specific bounds type) /// @note Typical size range: 2-8 parameters virtual std::vector<double> values() const = 0;Core/src/Surfaces/DiamondBounds.cpp (1)
26-30: More efficient, the values() method could be, hmmmm.Return by const reference instead of copying, we should consider. Performance benefits, this would bring.
-std::vector<double> DiamondBounds::values() const { - std::vector<double> valvector; - valvector.insert(valvector.begin(), m_values.begin(), m_values.end()); - return valvector; +const std::vector<double>& DiamondBounds::values() const { + return m_values; }Core/src/Surfaces/EllipseBounds.cpp (1)
30-34: Optimize the Force flow, we must!A more efficient path, I sense. Reserve the vector's capacity before insertion, we should.
std::vector<double> EllipseBounds::values() const { std::vector<double> valvector; + valvector.reserve(m_values.size()); valvector.insert(valvector.begin(), m_values.begin(), m_values.end()); return valvector; }Core/include/Acts/Surfaces/SurfaceConcept.hpp (1)
Line range hint
98-122: Strong with the Force, this concept is!A powerful addition to our type system, the RegularSurfaceConcept brings. Compile-time safety, it ensures. But remember, young Padawan, document the requirements in detail, you must.
Consider adding:
- Documentation comments for each required method
- Example implementation in the test suite
- Migration guide for existing surface types
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (2)
Line range hint
27-65: Hmmmm, well-structured this function is!Elegant handling of boundary cases, I see. Early returns for infinite tolerance and inside-box checks, performance they bring. Yet, a small optimization opportunity, there might be.
Consider this optimization, you should:
if (!tolerance.hasMetric(jacobianOpt.has_value())) { + // Compute squared distance first for early exit + double squaredDistance = detail::VerticesHelper::computeSquaredDistance( + point, lowerLeft, upperRight); + if (squaredDistance <= tolerance.squaredValue()) { + return true; + } closestPoint = detail::VerticesHelper::computeEuclideanClosestPointOnRectangle( point, lowerLeft, upperRight); }
Line range hint
89-90: Wise observation in TODO comment, I sense.A performance enhancement through bounding box optimization, suggested it is. Help implement this optimization, I can.
Shall I generate the implementation for the bounding box optimization, young padawan?
Core/include/Acts/Surfaces/RadialBounds.hpp (1)
102-105: Simple and clear, these binning methods are!Yet document their purpose in physics context, we should.
Add physics context to documentation:
- /// Return a reference radius for binning + /// Return a reference radius for binning in detector geometry + /// @note This value is used for organizing detector elements in R-phi coordinates double binningValueR() const final { return 0.5 * (get(eMinR) + get(eMaxR)); }Core/src/Surfaces/detail/AlignmentHelper.cpp (1)
Line range hint
15-91: Strong with the Force, this implementation is!Mathematical precision in rotation calculations, I sense. Yet clearer documentation for future Padawans, we need.
Add this documentation block before the implementation:
/// @brief Calculates derivatives of local axes with respect to rotation parameters /// @details This implementation follows the small-angle approximation for /// sequential rotations around local axes. The mathematical derivation /// assumes infinitesimal rotations (alpha, beta, gamma) around the /// original local (x, y, z) axes respectively. /// @param compositeRotation The combined rotation matrix /// @param relRotation The relative rotation matrix /// @return Tuple of derivatives for each local axisCore/src/Surfaces/IntersectionHelper2D.cpp (2)
Line range hint
47-119: Add mathematical documentation, you should!Complex mathematical derivations, this function contains. Help future maintainers, additional comments would. Document the ellipse intersection formula and its special cases, we must.
121-134: Validate phi range bounds, you must!Check that phiMin is less than phiMax, wisdom suggests. Add validation to prevent incorrect usage:
Intersection2D IntersectionHelper2D::intersectCircleSegment( double R, double phiMin, double phiMax, const Vector2& origin, const Vector2& dir) { + if (phiMin >= phiMax) { + return Intersection2D::invalid(); + } auto intersections = intersectCircle(R, origin, dir);Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1)
99-114: Optimize complex calculations, we should!Cache intermediate results for halfPhiSector and halfLengthY, wisdom suggests. Multiple uses of get() function calls, there are.
double halfPhiSector() const { - auto minHalfPhi = std::asin(get(eHalfLengthXminR) / get(eMinR)); - auto maxHalfPhi = std::asin(get(eHalfLengthXmaxR) / get(eMaxR)); + const double minR = get(eMinR); + const double maxR = get(eMaxR); + const double hxMinR = get(eHalfLengthXminR); + const double hxMaxR = get(eHalfLengthXmaxR); + auto minHalfPhi = std::asin(hxMinR / minR); + auto maxHalfPhi = std::asin(hxMaxR / maxR); return std::max(minHalfPhi, maxHalfPhi); }Core/src/Surfaces/CylinderBounds.cpp (2)
34-38: More efficient, your implementation could be, young padawan.Copy constructor of vector, unnecessary overhead it creates. Reserve and emplace_back, a more efficient path would be.
- std::vector<double> valvector; - valvector.insert(valvector.begin(), m_values.begin(), m_values.end()); - return valvector; + std::vector<double> valvector; + valvector.reserve(m_values.size()); + for (const auto& value : m_values) { + valvector.emplace_back(value); + } + return valvector;
Line range hint
115-161: Separate the bevel transformation logic, we should.In-place modification of vertices, risky it can be. A separate transformation function, cleaner it would be.
Extract bevel transformation logic:
std::vector<Vector3> applyBevelTransformation( std::vector<Vector3> vertices, const Transform3& transform, double bevelMinZ, double bevelMaxZ) const;Plugins/Json/src/SurfaceJsonConverter.cpp (1)
Line range hint
52-124: A map-based approach, consider you should.Long switch statements, difficult to maintain they are. A factory pattern with type mapping, more elegant it would be.
Consider implementing a factory pattern:
using SurfaceFactory = std::function<std::shared_ptr<Surface>(const nlohmann::json&)>; static const std::unordered_map<Surface::SurfaceType, std::unordered_map<SurfaceBounds::BoundsType, SurfaceFactory>> surfaceFactories;Core/src/Surfaces/PlaneSurface.cpp (1)
Line range hint
93-135: Clarify the conditions and extract magic numbers, we must.Complex logic, this method contains. Named constants and helper methods, clarity they would bring.
+ static constexpr double FULL_PHI_TOLERANCE = 1e-6; + bool isFullPhi(double halfPhiSector) { + return std::abs(halfPhiSector - std::numbers::pi) < FULL_PHI_TOLERANCE; + } + - bool coversFull = std::abs(vStore[EllipseBounds::eHalfPhiSector] - - std::numbers::pi) < s_epsilon; + bool coversFull = isFullPhi(vStore[EllipseBounds::eHalfPhiSector]);Core/include/Acts/Surfaces/AnnulusBounds.hpp (2)
110-113: Readability improve, we must.Complex expression in coversFullAzimuth(), harder to understand it makes. A named constant or intermediate variable, clearer it would be.
bool coversFullAzimuth() const final { - return (std::abs((get(eMinPhiRel) - get(eMaxPhiRel)) - std::numbers::pi) < - s_onSurfaceTolerance); + const double phiDifference = get(eMinPhiRel) - get(eMaxPhiRel); + const double fullCircleDifference = std::abs(phiDifference - std::numbers::pi); + return fullCircleDifference < s_onSurfaceTolerance; }
Line range hint
4-4: Missing tests, I sense. Address this, we must.Unit tests, crucial they are. Help you write them, I can.
Generate comprehensive test cases for this module, shall I?
Core/src/Geometry/CylinderVolumeBounds.cpp (1)
Line range hint
286-295: Error messages, more descriptive they should be.When invalid bevel angles detected are, clearer guidance the error messages should provide. The actual and allowed values, included they should be.
if (get(eBevelMinZ) != detail::radian_sym(get(eBevelMinZ))) { - throw std::invalid_argument("CylinderBounds: invalid bevel at min Z."); + throw std::invalid_argument("CylinderBounds: invalid bevel at min Z. Value " + + std::to_string(get(eBevelMinZ)) + " not within normalized range [-π, π]."); } if (get(eBevelMaxZ) != detail::radian_sym(get(eBevelMaxZ))) { - throw std::invalid_argument("CylinderBounds: invalid bevel at max Z."); + throw std::invalid_argument("CylinderBounds: invalid bevel at max Z. Value " + + std::to_string(get(eBevelMaxZ)) + " not within normalized range [-π, π]."); }Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp (1)
32-32: Strong with assertions, your tests have become!Better error handling through ThrowAssert.hpp, you have achieved. Catch problems early, this will. Robust testing practices, these are.
Consider using ThrowAssert in other test files too, for consistent error handling across the test suite, you should.
Core/src/Surfaces/Surface.cpp (2)
Line range hint
26-48: Consider move semantics for transform initialization, you should!- if (other.m_transform) { - m_transform = std::make_unique<Transform3>(*other.m_transform); - } + m_transform = other.m_transform ? + std::make_unique<Transform3>(std::move(*other.m_transform)) : nullptr;More efficient memory handling, this change brings. Optional but beneficial, it is.
Line range hint
325-376: Document the mathematical transformations, you must!Complex calculations these are. Add detailed comments explaining:
- The coordinate transformation matrix
- The normalization factor calculations
- The path derivatives
Clearer understanding for future Padawans, this will bring.
Core/src/Surfaces/ConeSurface.cpp (1)
158-177: Optimize normal calculations, we can!Cache trigonometric calculations, you should:
+ const double cosPhi = std::cos(phi); + const double sinPhi = std::sin(phi); - Vector3 localNormal(cos(phi) * cosAlpha, sin(phi) * cosAlpha, sgn * sinAlpha); + Vector3 localNormal(cosPhi * cosAlpha, sinPhi * cosAlpha, sgn * sinAlpha);Small but faster, this change will be.
Core/src/Surfaces/AnnulusBounds.cpp (1)
105-118: More specific error messages, provide you must!- throw std::invalid_argument("AnnulusBounds: invalid radial setup."); + throw std::invalid_argument( + fmt::format("AnnulusBounds: invalid radial setup. minR={}, maxR={}, " + "difference={}", get(eMinR), get(eMaxR), + std::abs(get(eMinR) - get(eMaxR))));Easier debugging in the future, this will enable.
Core/src/Surfaces/DiscSurface.cpp (1)
Line range hint
154-194: Documentation, this complex geometry requires!Clear the code may be, but mysterious the calculations remain. Add comments explaining the geometry transformations and face mesh generation, we should.
Add explanatory comments like these:
Polyhedron DiscSurface::polyhedronRepresentation( const GeometryContext& gctx, unsigned int quarterSegments) const { + // Generate vertices for the disc surface representation std::vector<Vector3> vertices; bool fullDisc = m_bounds->coversFullAzimuth(); bool toCenter = m_bounds->rMin() < s_onSurfaceTolerance; + // Determine the type of polyhedron to generate based on bounds bool exactPolyhedron = (m_bounds->type() == SurfaceBounds::eDiscTrapezoid);Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp (1)
Line range hint
42-1300: Strong with the Force, these tests are!Comprehensive coverage, I sense. Construction, properties, assignment, and merging - all aspects tested they are. Well-structured and thorough, the test suite is. Edge cases and error conditions, forgotten they are not.
A suggestion for future padawans, I have. Documentation of test case purposes, enhanced it could be:
+ // Tests the construction of DiscSurface objects with various parameters BOOST_AUTO_TEST_CASE(DiscSurfaceConstruction) {Core/src/Surfaces/LineSurface.cpp (5)
27-42: Hmm, well-crafted these constructors are!Sound implementation practices I see - proper memory management with std::make_shared and move semantics you use. Null check protection, wise it is! Yet, a small suggestion I have - document the expected parameter ranges for radius and halez, helpful it would be.
43-56: Strong with the Force, these copy operations are!Follow the Rule of Three correctly, you do. Yet, consider the path to modern C++ you might -
= defaultfor these operations if no custom behavior needed it is. Simplify the code, this would.-LineSurface::LineSurface(const LineSurface& other) - : GeometryObject(), Surface(other), m_bounds(other.m_bounds) {} +LineSurface::LineSurface(const LineSurface& other) = default; -LineSurface& LineSurface::operator=(const LineSurface& other) { - if (this != &other) { - Surface::operator=(other); - m_bounds = other.m_bounds; - } - return *this; -} +LineSurface& LineSurface::operator=(const LineSurface& other) = default;
Line range hint
142-186: Complex calculations of intersection, I sense!Well-handled edge cases are, but document the mathematical derivation you should. Future Padawans, understand this code better they would.
Add documentation explaining:
- The geometric interpretation of
denom- Why
toleranceis used as a threshold for parallel vectors- The boundary check's geometric meaning
Line range hint
225-283: Similar patterns in the Force, I sense!Common calculations between
freeToPathDerivativeandalignmentToPathDerivative, repeated they are. Extract shared computations to private helper method, you should.Consider creating a private helper:
private: struct PathCalculationParams { Vector3 pcRowVec; Vector3 localZAxis; double pz; double dz; double norm; }; PathCalculationParams computePathParams(const GeometryContext& gctx, const Vector3& position, const Vector3& direction) const;
Line range hint
284-295: Efficient transformation, seek we must!Calculate localPhi only once and cache the trigonometric values, you should. Multiple calculations, avoided they can be.
double localPhi = VectorHelpers::phi(localPosition); +double cosPhi = std::cos(localPhi); +double sinPhi = std::sin(localPhi); ActsMatrix<2, 3> loc3DToLocBound = ActsMatrix<2, 3>::Zero(); -loc3DToLocBound << std::cos(localPhi), std::sin(localPhi), 0, 0, 0, 1; +loc3DToLocBound << cosPhi, sinPhi, 0, 0, 0, 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
Core/include/Acts/Surfaces/AnnulusBounds.hpp(3 hunks)Core/include/Acts/Surfaces/BoundaryTolerance.hpp(0 hunks)Core/include/Acts/Surfaces/ConeBounds.hpp(2 hunks)Core/include/Acts/Surfaces/ConeSurface.hpp(1 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp(0 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp(0 hunks)Core/include/Acts/Surfaces/CylinderBounds.hpp(2 hunks)Core/include/Acts/Surfaces/CylinderSurface.hpp(0 hunks)Core/include/Acts/Surfaces/DiamondBounds.hpp(0 hunks)Core/include/Acts/Surfaces/DiscSurface.hpp(0 hunks)Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp(2 hunks)Core/include/Acts/Surfaces/EllipseBounds.hpp(0 hunks)Core/include/Acts/Surfaces/InfiniteBounds.hpp(1 hunks)Core/include/Acts/Surfaces/LineBounds.hpp(0 hunks)Core/include/Acts/Surfaces/LineSurface.hpp(1 hunks)Core/include/Acts/Surfaces/PerigeeSurface.hpp(1 hunks)Core/include/Acts/Surfaces/PlaneSurface.hpp(0 hunks)Core/include/Acts/Surfaces/RadialBounds.hpp(1 hunks)Core/include/Acts/Surfaces/RectangleBounds.hpp(2 hunks)Core/include/Acts/Surfaces/RegularSurface.hpp(1 hunks)Core/include/Acts/Surfaces/StrawSurface.hpp(1 hunks)Core/include/Acts/Surfaces/Surface.hpp(0 hunks)Core/include/Acts/Surfaces/SurfaceArray.hpp(1 hunks)Core/include/Acts/Surfaces/SurfaceBounds.hpp(1 hunks)Core/include/Acts/Surfaces/SurfaceConcept.hpp(1 hunks)Core/include/Acts/Surfaces/SurfaceMergingException.hpp(1 hunks)Core/include/Acts/Surfaces/TrapezoidBounds.hpp(1 hunks)Core/include/Acts/Surfaces/detail/AlignmentHelper.hpp(0 hunks)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(1 hunks)Core/include/Acts/Surfaces/detail/MergeHelper.hpp(0 hunks)Core/src/Geometry/CylinderVolumeBounds.cpp(1 hunks)Core/src/Surfaces/AnnulusBounds.cpp(8 hunks)Core/src/Surfaces/BoundaryTolerance.cpp(0 hunks)Core/src/Surfaces/ConeBounds.cpp(3 hunks)Core/src/Surfaces/ConeSurface.cpp(10 hunks)Core/src/Surfaces/ConvexPolygonBounds.cpp(3 hunks)Core/src/Surfaces/CurvilinearSurface.cpp(0 hunks)Core/src/Surfaces/CylinderBounds.cpp(6 hunks)Core/src/Surfaces/CylinderSurface.cpp(14 hunks)Core/src/Surfaces/DiamondBounds.cpp(4 hunks)Core/src/Surfaces/DiscSurface.cpp(12 hunks)Core/src/Surfaces/DiscTrapezoidBounds.cpp(5 hunks)Core/src/Surfaces/EllipseBounds.cpp(3 hunks)Core/src/Surfaces/IntersectionHelper2D.cpp(4 hunks)Core/src/Surfaces/LineBounds.cpp(3 hunks)Core/src/Surfaces/LineSurface.cpp(9 hunks)Core/src/Surfaces/PerigeeSurface.cpp(3 hunks)Core/src/Surfaces/PlaneSurface.cpp(3 hunks)Core/src/Surfaces/RadialBounds.cpp(2 hunks)Core/src/Surfaces/RectangleBounds.cpp(2 hunks)Core/src/Surfaces/StrawSurface.cpp(2 hunks)Core/src/Surfaces/Surface.cpp(12 hunks)Core/src/Surfaces/SurfaceArray.cpp(2 hunks)Core/src/Surfaces/TrapezoidBounds.cpp(6 hunks)Core/src/Surfaces/detail/AlignmentHelper.cpp(2 hunks)Core/src/Surfaces/detail/AnnulusBoundsHelper.cpp(2 hunks)Core/src/Surfaces/detail/MergeHelper.cpp(2 hunks)Core/src/Surfaces/detail/VerticesHelper.cpp(4 hunks)Plugins/Json/src/SurfaceJsonConverter.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/ConeSurfaceTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/ConvexPolygonBoundsTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/CylinderBoundsTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/PolyhedronSurfacesTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/RadialBoundsTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/StrawSurfaceTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/SurfaceArrayTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/SurfaceIntersectionTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/SurfaceLocalToGlobalRoundtripTests.cpp(0 hunks)Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp(0 hunks)
💤 Files with no reviewable changes (23)
- Core/include/Acts/Surfaces/detail/AlignmentHelper.hpp
- Tests/UnitTests/Core/Surfaces/SurfaceIntersectionTests.cpp
- Core/include/Acts/Surfaces/BoundaryTolerance.hpp
- Core/src/Surfaces/BoundaryTolerance.cpp
- Tests/UnitTests/Core/Surfaces/RadialBoundsTests.cpp
- Core/src/Surfaces/CurvilinearSurface.cpp
- Tests/UnitTests/Core/Surfaces/PolyhedronSurfacesTests.cpp
- Tests/UnitTests/Core/Surfaces/SurfaceLocalToGlobalRoundtripTests.cpp
- Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp
- Tests/UnitTests/Core/Surfaces/CylinderBoundsTests.cpp
- Tests/UnitTests/Core/Surfaces/ConvexPolygonBoundsTests.cpp
- Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp
- Core/include/Acts/Surfaces/DiscSurface.hpp
- Core/include/Acts/Surfaces/LineBounds.hpp
- Tests/UnitTests/Core/Surfaces/StrawSurfaceTests.cpp
- Core/include/Acts/Surfaces/CylinderSurface.hpp
- Tests/UnitTests/Core/Surfaces/SurfaceArrayTests.cpp
- Core/include/Acts/Surfaces/detail/MergeHelper.hpp
- Core/include/Acts/Surfaces/PlaneSurface.hpp
- Core/include/Acts/Surfaces/EllipseBounds.hpp
- Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp
- Core/include/Acts/Surfaces/DiamondBounds.hpp
- Core/include/Acts/Surfaces/Surface.hpp
✅ Files skipped from review due to trivial changes (4)
- Core/include/Acts/Surfaces/RegularSurface.hpp
- Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
- Core/include/Acts/Surfaces/SurfaceMergingException.hpp
- Core/include/Acts/Surfaces/SurfaceArray.hpp
🔇 Additional comments (61)
Core/src/Surfaces/detail/VerticesHelper.cpp (2)
Line range hint 18-71: Hmmmm... Clean and precise, this code is!
Strong with the Force, this implementation is. Thorough validation it has, and modern C++ features it embraces. Pleased with the namespace simplification, I am.
16-17: Wise, these namespace changes are!
Clean and consistent, the namespace structure has become. Aligns with the Force... err, I mean, the PR objectives, it does!
Also applies to: 140-140
Core/src/Surfaces/RectangleBounds.cpp (4)
15-15: Wise addition of stdexcept, this is!
For the throwing of exceptions, necessary this header is. Properly placed in the include hierarchy, it stands.
44-47: Elegant simplicity, this method shows!
Delegate to the helper function, it does. Clean and efficient, the implementation is.
50-53: Wisdom in simplicity, this method demonstrates!
Counter-clockwise the vertices flow, like the cycle of life itself. Clear and purposeful, each line stands.
55-57: Truth in self-reference, this method speaks!
Its own bounding box, a rectangle is. Wisdom in this simplicity lies.
Core/src/Surfaces/RadialBounds.cpp (5)
19-19: Wise addition of stdexcept header, this is!
For the new exception throwing in checkConsistency(), necessary this include becomes. Clear and purposeful, the dependency management is.
45-51: Clear and concise, the shifted implementation is.
Good use of the detail::radian_sym helper, I observe. The Force flows smoothly through this function.
53-58: Elegant use of detail::insideAlignedBox, mmmmm.
Clean and efficient implementation, this is. The boundary check helper, well utilized it is.
61-64: Wisdom in delegating to VerticesHelper, there is.
Reuse of common functionality through helper class, a path to maintainable code this is.
Line range hint 66-74: Stream formatting, carefully handled it is.
But verify the precision restoration, we must. After setting precision to 7, properly restored to default it should be.
Run this command to check precision handling in other files:
✅ Verification successful
The Force guides me to check how precision is restored before returning the stream. Let us see if other Bounds follow similar path.
Proper handling of stream precision, I observe.
In all Bounds classes, restored to default precision with setprecision(-1) before returning, the stream is. A consistent pattern across the codebase, this forms. The Force is strong with this implementation, hmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar toStream implementations to verify precision handling
rg -A 3 'setprecision\(7\)'
Length of output: 3925
Script:
#!/bin/bash
# Search for precision restoration in toStream implementations
rg -B 5 'setprecision\(-1\)|setprecision\(.*\).*return'
Length of output: 6373
Core/src/Surfaces/TrapezoidBounds.cpp (1)
Line range hint 18-147: Strong with the Force, this implementation is!
Clean and well-structured, the code has become. Namespace cleanup objectives, achieved they are. Error handling and geometric calculations, precise they remain.
Core/src/Surfaces/DiscTrapezoidBounds.cpp (1)
19-21: Hmm, clean and organized, the includes now are!
Added stdexcept header and wrapped implementation in Acts namespace, a wise choice this is. Simplified the code structure, it has.
Core/include/Acts/Surfaces/TrapezoidBounds.hpp (2)
Line range hint 23-45: Wisdom in simplicity, I sense. Removed, the explicit deletions are.
Follow modern C++ practices, we do. Let the compiler generate what it must, we shall. Clean and maintainable, the code becomes.
10-19: Hmm, clean and organized, the includes have become.
Removed, the unnecessary headers are. Yet verify we must, that mathematical operations in the implementation, the removal of cmath affects not.
✅ Verification successful
Safe to remove headers, it is. Wisdom in simplification, there is.
Through the Force, seen I have that std::abs in implementation file only is used, through Algebra.hpp header provided it is. The cmath header, needed directly it is not. Mathematical operations simple they are, through existing includes supported they remain.
In the implementation file:
- Only std::abs for boundary checks used it is
- No complex mathematical operations requiring cmath, found there are
- Through Algebra.hpp, necessary mathematical support provided it is
Clean and sufficient, the current includes are. Removed headers, impact they do not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for math operations in the implementation
rg -l "std::(sin|cos|sqrt|pow|abs)" --type cpp Core/include/Acts/Surfaces/TrapezoidBounds
Length of output: 174
Script:
#!/bin/bash
# Search for math operations in trapezoid bounds implementation, with broader pattern
rg -l "(std::)?(sin|cos|sqrt|pow|abs|fabs|M_PI)" --type cpp "trapezoid"
# Find the actual path of TrapezoidBounds
fd -I "TrapezoidBounds"
# Look for math operations in the implementation
ast-grep --pattern 'std::$_($$$)'
Length of output: 514
Script:
#!/bin/bash
# Check content of implementation file for math operations
rg "(std::)?(sin|cos|sqrt|pow|abs|fabs|M_PI)" Core/src/Surfaces/TrapezoidBounds.cpp -A 2
# Check if cmath is used in implementation
rg "#include.*cmath" Core/src/Surfaces/TrapezoidBounds.cpp
# Check content of header for any math operations
cat Core/include/Acts/Surfaces/TrapezoidBounds.hpp
Length of output: 5375
Core/include/Acts/Surfaces/RectangleBounds.hpp (2)
66-70: Hmmmm... Clean and efficient, these implementations are!
Simple and direct, the path to wisdom is. These inline implementations, clarity they bring. The Force flows smoothly through this code.
Line range hint 1-124: Hmmmm... The Force of constexpr, consider we must!
Strong the class design is, yet stronger it could become. Through the Force, I sense potential for compile-time optimizations:
- Mark simple getters as constexpr, we should
- Compile-time consistency checks, possible they are
- Performance benefits, these changes bring
Search for similar patterns in the codebase, we must:
Core/src/Surfaces/detail/MergeHelper.cpp (2)
11-13: Wisdom in these changes, I sense!
Clean and precise, these modifications are. Added includes, essential they are, and simplified namespace usage, clarity it brings. Hmmmm.
Also applies to: 22-22
Line range hint 24-33: Verify the tolerance usage across the codebase, we must!
Consistent handling of tolerances, critical it is. Check we must, that other parts of the codebase align with this approach.
Run this script, we shall:
✅ Verification successful
Consistent tolerance usage across the codebase, I sense!
Searched the Force has revealed:
- The
s_onSurfaceToleranceconstant, widely used it is across geometric operations - In phi-related calculations, including
difference_periodic, consistent tolerance handling exists - The MergeHelper's approach, aligned with the codebase's standard practices it is
Through the Force, see I do that:
- Surface intersection checks
- Boundary condition validations
- Geometric transformations
All follow similar tolerance patterns, they do!
No disturbance in the Force regarding tolerance usage, there is. Consistent with the rest of the codebase, this implementation is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for tolerance usage patterns in phi-related calculations
# Search for similar tolerance checks in phi calculations
ast-grep --pattern 'std::abs($_ - std::numbers::pi / $_) < tolerance'
# Search for other uses of tolerance in periodic calculations
rg -A 2 'difference_periodic.*tolerance'
Length of output: 114
Script:
#!/bin/bash
# Hmmmm, search deeper we must! Different approach, we shall try.
# Search for tolerance usage in phi-related calculations
rg -B 2 -A 2 'std::abs.*phi.*tolerance|tolerance.*phi'
# Look for tolerance declarations and definitions
rg -B 2 -A 2 'tolerance\s*=|tolerance\s*\('
# Find files containing both phi and tolerance
rg --files-with-matches 'phi' | xargs rg --files-with-matches 'tolerance'
# Search for difference_periodic function usage
ast-grep --pattern 'difference_periodic($$$)'
Length of output: 45477
Core/include/Acts/Surfaces/InfiniteBounds.hpp (1)
Line range hint 23-42: Wise decision, the removal of explicit default constructor and destructor is!
Clean and modern, the code becomes. Implicitly-defined special members by the compiler, sufficient they are. The Force flows better through simplified code, it does!
Core/src/Surfaces/LineBounds.cpp (1)
Line range hint 17-43: Wise cleanup of namespace prefixes, this is!
Clean and readable, the code has become. Proper namespace encapsulation maintained, it is. The Force flows smoothly through simplified declarations!
Core/src/Surfaces/detail/AnnulusBoundsHelper.cpp (3)
16-17: Simplified namespace declaration, approve this change I do!
Cleaner and more maintainable, the code becomes. Follows modern C++ practices, it does.
18-20: Removed redundant Acts:: prefix, wise decision this is!
Simplified the method signature has become, yet its purpose remains clear. Return type and parameters, well-chosen they are.
Line range hint 33-39: Verify the error handling path, we must.
Critical error path this is, when bound lines do not equal two. Test coverage for this scenario, essential it becomes.
Core/src/Surfaces/PerigeeSurface.cpp (1)
20-22: Constructor implementations, elegant they have become!
Clean and consistent, the constructors now are. Removed redundant namespace qualifiers, we have. Inheritance chain, properly maintained it is.
Also applies to: 23-24, 26-27, 29-31
Core/include/Acts/Surfaces/SurfaceBounds.hpp (2)
Line range hint 28-43: Well-organized enum values, clarity they bring!
Comprehensive list of bound types, maintained it is. Sequential numbering, future additions it supports. Documentation, clear it remains.
Line range hint 76-84: Equality comparison, efficient implementation it has!
Address comparison first, wise optimization it is. Type and values comparison, sufficient they are for equality check.
Core/src/Surfaces/SurfaceArray.cpp (1)
Line range hint 17-78: Clean and elegant, these namespace changes are!
Removed the redundant Acts:: prefix from method definitions, you have. Improved readability without changing functionality, this does. A wise choice in code cleanup, mmm, yes!
Core/include/Acts/Surfaces/PerigeeSurface.hpp (1)
Line range hint 1-90: Hmm, changes mentioned in summary, visible they are not.
In the provided code segment, see the actual changes, I cannot. Skip this review, we must.
Core/src/Surfaces/DiamondBounds.cpp (2)
32-40: Strong with the Force, this validation is!
Wise decision to add consistency checks, you have made. Clear error messages and proper validation of diamond shape constraints, I see. Prevent invalid states, this will!
Line range hint 42-57: Clean and consistent, the namespace usage now is.
Removed redundant Acts:: prefix from method definitions, maintaining clarity without verbosity. Approve of this cleanup, I do!
Also applies to: 58-69, 71-88
Core/src/Surfaces/StrawSurface.cpp (1)
Line range hint 21-89: Clean and wise, these changes are!
Removed the redundant Acts:: namespace qualifiers, you have. Improved readability without affecting functionality, this does. A path to cleaner code, you follow!
Core/include/Acts/Surfaces/StrawSurface.hpp (1)
83-86: Inline implementations, a wise choice they are!
Simple methods type() and name(), inline they now become. Compiler's work, easier this makes. Performance benefits, subtle they may be, but cleaner code structure, undeniable it is!
Core/src/Surfaces/ConeBounds.cpp (2)
44-48: Simple yet effective, this new method is!
The values() method, a clean way to access internal parameters it provides. Copy of the values, safer than direct access it is!
Line range hint 67-77: Wisdom in handling edge cases, I sense!
Special handling of normal numbers through std::isnormal, wise it is. Prevents undefined behavior when radius approaches zero, it does.
Core/src/Surfaces/EllipseBounds.cpp (1)
21-25: Wise simplification of namespace, this is!
Clean and elegant, the namespace usage has become. Through the Force, we sense improved readability with using declarations for perp and phi.
Core/include/Acts/Surfaces/ConeBounds.hpp (1)
74-74: Inline with the Force, these methods are!
Simple and efficient, these inline implementations be. Performance benefits, they bring.
Also applies to: 100-100, 103-103
Core/include/Acts/Surfaces/RadialBounds.hpp (1)
81-99: Clean and efficient, these inlined methods are!
Consistent use of get() method, I observe. Yet verify the impact on binary size, we must.
✅ Verification successful
Inlining these methods, safe it is!
Through the Force, observed the usage patterns I have. No binary size concerns exist, as these methods:
- Simple accessors they are, returning values from an array
- Used consistently across the codebase through the
get()method - In test files and implementation code alike, properly utilized they are
The inlining decision, beneficial it is:
- Improved readability it brings
- Performance impact negligible or positive it shall be
- No complexity added to the codebase, hmm
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if inlining these methods impacts binary size
echo "Searching for RadialBounds usage across codebase..."
rg -l "RadialBounds::" | while read -r file; do
echo "=== $file ==="
rg -A 2 "RadialBounds::" "$file"
done
Length of output: 14646
Core/src/Surfaces/IntersectionHelper2D.cpp (1)
Line range hint 17-46: Approve the namespace change and intersectSegment implementation, I do!
Clean and efficient, the namespace declaration is. Well-structured intersection logic, the function maintains. Proper error handling with epsilon checks, it has.
Core/include/Acts/Surfaces/CylinderBounds.hpp (2)
108-108: Approve the inline implementation, I do!
Simple and clear, this change is. Performance benefit from inlining, we may receive.
141-141: Approve the type alias usage, I do!
More consistent with project conventions, this change is. Clearer intent through type alias, it shows.
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (2)
121-123: Approve the bounds checking implementation, I do!
Clear and efficient, this implementation is. Proper tolerance handling, it maintains.
126-129: Approve the binning value implementations, I do!
Simple and purposeful, these methods are. Efficient access to internal values, they provide.
Core/src/Surfaces/CylinderBounds.cpp (1)
Line range hint 162-184: Wise and thorough, these consistency checks are.
Clear error messages and comprehensive validation, you have provided. The Force, strong with this implementation it is.
Core/src/Surfaces/PlaneSurface.cpp (2)
33-51: Well-implemented constructors, these are.
Clear initialization paths and proper validation, you have provided. The Force guides your implementation.
Line range hint 162-188: Clean and efficient, this intersection implementation is.
Well-separated concerns and proper error handling, you have achieved. The Force flows smoothly through this code.
Core/include/Acts/Surfaces/AnnulusBounds.hpp (1)
73-75: Simple and clear, this implementation is.
The type() method, well-defined it now stands, returning the correct enumeration directly.
Tests/UnitTests/Core/Surfaces/ConeSurfaceTests.cpp (1)
25-25: Wise addition, this include is.
Enhanced error handling through ThrowAssert.hpp, better test diagnostics it brings.
Core/src/Geometry/CylinderVolumeBounds.cpp (1)
22-22: Appropriate include for periodic calculations, this is.
The detail/periodic.hpp, necessary functionality it provides for angle normalization.
Core/include/Acts/Surfaces/ConeSurface.hpp (1)
41-41: Clean and clear, the documentation now is!
Improved readability through proper formatting, this change brings. Continue with such practices, you should.
Core/include/Acts/Surfaces/LineSurface.hpp (1)
10-10: Wise spacing between includes, this is!
Better organization of header blocks, you have achieved. Clear separation, brings clarity it does.
Core/src/Surfaces/Surface.cpp (1)
20-24: Clean and proper, the namespace declaration and type names array are!
Simplified namespace usage, you have achieved. Approve this change, I do.
Core/src/Surfaces/DiscSurface.cpp (3)
29-29: Hmm, clean and organized, these changes are!
Added include for periodic operations and simplified namespace access through using directives, we have. Improve code readability without compromising functionality, these changes do.
Also applies to: 38-39
Line range hint 372-546: Strong with the Force, this merging implementation is!
Proper error handling and logging, it has. Clear validation of merging conditions, it maintains. Well-structured and robust, the code is.
147-152:
Guard against the dark side of null pointers, we must!
A potential crash lurks in the bounds() method when m_bounds is null. Safer handling, we need.
Apply this change to protect against the dark side:
const SurfaceBounds& DiscSurface::bounds() const {
- if (m_bounds) {
- return (*(m_bounds.get()));
- }
- return s_noBounds;
+ return m_bounds ? *m_bounds : s_noBounds;
}Likely invalid or redundant comment.
Core/src/Surfaces/CylinderSurface.cpp (1)
37-38: Clean and consistent, these namespace changes are!
Follow the same pattern as DiscSurface.cpp, these modifications do. Improve readability without affecting functionality, they do.
Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp (1)
30-30: Strong with testing, this code is!
Added ThrowAssert.hpp for better error handling in tests, we have. Comprehensive coverage of surface functionality, these tests maintain.
Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp (1)
31-31: Wise changes to the includes, I sense!
Clean and purposeful, the addition of ThrowAssert.hpp is, while removed the unused TrackParametrization.hpp becomes. Balance to the Force, these changes bring.
Core/src/Surfaces/LineSurface.cpp (3)
Line range hint 188-224: Strong with mathematics, this Jacobian implementation is!
Well documented with white paper reference, it is. Proper assertions guard the calculations, they do. Clean and maintainable, the code remains.
296-299: Simple and elegant, this method is!
Efficient access to the transform's z-axis it provides. No improvements needed, I sense.
Line range hint 58-98: Careful with the coordinate transformations, we must be!
Mathematically sound, your calculations are. Yet, validate the direction vector's normalization, you should. Undefined behavior lurks when zero vectors pass, it does.
Vector3 LineSurface::localToGlobal(const GeometryContext& gctx,
const Vector2& lposition,
const Vector3& direction) const {
+ throw_assert(direction.norm() > 0, "Direction vector must not be zero");
Vector3 unitZ0 = lineDirection(gctx);
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
Core/include/Acts/Surfaces/DiamondBounds.hpp (1)
Line range hint
41-54: Consistency check missing in array constructor, hmm!Dangerous path this is! First constructor checks consistency, but array constructor does not. To the dark side of invalid objects, this path leads.
Add consistency check we must:
DiamondBounds(const std::array<double, eSize>& values) noexcept(false) : m_values(values), m_boundingBox( Vector2{-(*std::max_element(values.begin(), values.begin() + 2)), -values[eHalfLengthYneg]}, Vector2{*std::max_element(values.begin(), values.begin() + 2), - values[eHalfLengthYpos]}) {} + values[eHalfLengthYpos]}) { + checkConsistency(); + }Core/src/Surfaces/DiscTrapezoidBounds.cpp (1)
Check the argument under square root, you must!
Hmm, observed I have that in
IntersectionHelper2D.cpp, checks for numerical stability exist they do:if (std::abs(D) < s_epsilon)But in
DiscTrapezoidBounds, no such protection exists! A dangerous path, this is. Before taking square root, ensure we must that:
get(eMinR) * get(eMinR) >= get(eHalfLengthXminR) * get(eHalfLengthXminR)get(eMaxR) * get(eMaxR) >= get(eHalfLengthXmaxR) * get(eHalfLengthXmaxR)The Force suggests this fix:
auto minRSquared = get(eMinR) * get(eMinR); auto xminRSquared = get(eHalfLengthXminR) * get(eHalfLengthXminR); if (minRSquared < xminRSquared) { throw std::domain_error("Invalid geometry: minR too small for halfLengthXminR"); } auto ymin = std::sqrt(minRSquared - xminRSquared);Similar protection for
m_ymaxcalculation in constructor, needed it is.🔗 Analysis chain
Line range hint
81-93: Guard against numerical instability, we must.In vertex calculations, floating-point precision issues lurk they might. Add checks for numerical stability, wise it would be.
auto ymin = std::sqrt(get(eMinR) * get(eMinR) - - get(eHalfLengthXminR) * get(eHalfLengthXminR)); + std::min(get(eHalfLengthXminR) * get(eHalfLengthXminR), + get(eMinR) * get(eMinR)));
Let me examine the numerical stability practices in geometric calculations, I shall.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar geometric calculations in codebase ast-grep --pattern 'std::sqrt($x * $x - $y * $y)' # Look for existing numerical stability handling rg -l 'std::numeric_limits|std::epsilon|tolerance' Core/Length of output: 6897
Script:
#!/bin/bash # Search for numerical stability checks in geometric calculations rg -A 5 'std::sqrt.*\(' Core/src/Surfaces/ # Check if there are any existing stability checks in DiscTrapezoidBounds rg -A 5 'std::sqrt' Core/src/Surfaces/DiscTrapezoidBounds.cpp # Look for similar bounds calculations in other geometric classes ast-grep --pattern 'auto $_ = std::sqrt($x)'Length of output: 4185
Core/src/Surfaces/DiscSurface.cpp (1)
Line range hint
274-308: Boundary check optimization, needed it is!In
intersectmethod, multiple matrix operations performed are. Cache intermediate results, we should.- const Vector3 vecLocal(intersection.position() - tMatrix.block<3, 1>(0, 3)); - const Vector2 lcartesian = tMatrix.block<3, 2>(0, 0).transpose() * vecLocal; + const auto& translation = tMatrix.block<3, 1>(0, 3); + const auto& rotation = tMatrix.block<3, 2>(0, 0); + const Vector3 vecLocal = intersection.position() - translation; + const Vector2 lcartesian = rotation.transpose() * vecLocal;
♻️ Duplicate comments (1)
Core/src/Surfaces/EllipseBounds.cpp (1)
65-66:⚠️ Potential issueCareful with division by zero, we must remain!
When tolerance equals radius, divide by zero we might. Previous wisdom shared, yet unaddressed it remains.
Apply this fix, we must:
- (1 < (square(lposition[eBoundLoc0] / (get(eInnerRx) - tol0)) + - square(lposition[eBoundLoc1] / (get(eOuterRx) - tol0)))); + (get(eInnerRx) <= tol0 || get(eOuterRx) <= tol0 || + 1 < (square(lposition[eBoundLoc0] / std::max(get(eInnerRx) - tol0, 1e-10)) + + square(lposition[eBoundLoc1] / std::max(get(eOuterRx) - tol0, 1e-10))));
🧹 Nitpick comments (19)
Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp (1)
Line range hint
1-190: Strong in the Force, this code structure is. Yet improvements, suggest I must.Consider these architectural insights, young Padawan:
Exception specifications clear and purposeful they are, but document them better we should. Help future Jedi understand when the Force may be disturbed, this will.
Template specialization between fixed and dynamic vertices, elegant it is. Yet, consider we must:
- Document performance implications between fixed and dynamic versions
- Guide users when to choose each path
Small optimization with boost::small_vector, clever it is. But document the chosen size of 10, we should.
Would you like assistance in enhancing the documentation, hmm?
Core/src/Surfaces/LineBounds.cpp (3)
23-29: Improve error messages and validation logic, we must!Two improvements, I sense:
- For radius check, more specific error message needed when r < 0
- For radius check, zero value should also be rejected
Apply this change, you should:
- if (get(eR) < 0.) { - throw std::invalid_argument("LineBounds: zero radius."); + if (get(eR) <= 0.) { + throw std::invalid_argument("LineBounds: zero or negative radius.");
Line range hint
32-39: Alignment of parameters, improve we should!Consistent indentation, maintain we must. One space too many in the function declaration, I see.
- const BoundaryTolerance& boundaryTolerance) const { + const BoundaryTolerance& boundaryTolerance) const {
Line range hint
42-50: Remove Acts:: prefix from output string, we should!Consistent with PR objectives of namespace cleanup, this change would be.
- sl << "Acts::LineBounds: (radius, halflengthInZ) = "; + sl << "LineBounds: (radius, halflengthInZ) = ";Core/include/Acts/Surfaces/LineBounds.hpp (1)
45-45: Consider moving type() implementation to source file, hmm!Though simple the implementation may be, consistency with other methods we should maintain. To source file, this method could move.
Move implementation to LineBounds.cpp, you could:
// In LineBounds.cpp BoundsType LineBounds::type() const { return SurfaceBounds::eLine; }Core/src/Surfaces/EllipseBounds.cpp (1)
Line range hint
88-98: Remove hardcoded namespace prefix from output, we should!Contradicts our cleanup efforts, the hardcoded "Acts::" in output string does. Consistency in all places, we must maintain.
- sl << "Acts::EllipseBounds: (innerRadius0, outerRadius0, innerRadius1, " + sl << "EllipseBounds: (innerRadius0, outerRadius0, innerRadius1, "Core/include/Acts/Surfaces/EllipseBounds.hpp (1)
82-83: Clearer warning message, provide we must!Vague, the current warning is. More specific about limitations of tolerance-based checks, we should be.
- /// @warning This **only** works for tolerance-based checks + /// @warning This method only supports boundary checks with explicit tolerance values. + /// Other types of boundary checks will result in an exception.Core/include/Acts/Surfaces/ConeBounds.hpp (1)
119-120: Documentation improvement, appreciate I do!Clearer understanding of the shifted method's purpose, this brings. Yet, consider adding parameter description, we should.
/// Private helper function to shift a local 2D position /// /// Shift r-phi coordinate to be centered around the average phi. /// - /// @param lposition The original local position + /// @param lposition The original local position in (r, phi) coordinatesCore/include/Acts/Surfaces/RadialBounds.hpp (1)
97-99: Bounds check implementation, clear and efficient it is!Tolerance handling, well-implemented it is. Yet, consider documenting tolerance's unit, we should.
- /// given the a tolerance + /// given a tolerance in the same unit as R (typically mm)Core/include/Acts/Surfaces/TrapezoidBounds.hpp (1)
54-56: Consider documentation for values() method, we should.Missing documentation for the values() method, I sense. Add it, we must.
+ /// Return the bound values as dynamically sized vector + /// + /// @return Vector containing the internal values in the order defined by BoundValues enum std::vector<double> values() const final;Core/src/Geometry/CutoutCylinderVolumeBounds.cpp (1)
35-47: Improve readability of validation checks, we must!Complex conditions, these are. Break them down into named variables, we should, for clarity to prevail.
Consider this path to enlightenment:
void CutoutCylinderVolumeBounds::checkConsistency() noexcept(false) { - if (get(eMinR) < 0. || get(eMedR) <= 0. || get(eMaxR) <= 0. || - get(eMinR) >= get(eMedR) || get(eMinR) >= get(eMaxR) || - get(eMedR) >= get(eMaxR)) { + bool validRadii = get(eMinR) >= 0. && get(eMedR) > 0. && get(eMaxR) > 0.; + bool properOrdering = get(eMinR) < get(eMedR) && get(eMedR) < get(eMaxR); + + if (!validRadii || !properOrdering) { throw std::invalid_argument( "CutoutCylinderVolumeBounds: invalid radial input."); } - if (get(eHalfLengthZ) <= 0 || get(eHalfLengthZcutout) <= 0. || - get(eHalfLengthZcutout) > get(eHalfLengthZ)) { + bool validLengths = get(eHalfLengthZ) > 0 && get(eHalfLengthZcutout) > 0.; + bool properNesting = get(eHalfLengthZcutout) <= get(eHalfLengthZ); + + if (!validLengths || !properNesting) { throw std::invalid_argument( "CutoutCylinderVolumeBounds: invalid longitudinal input."); }Core/include/Acts/Surfaces/AnnulusBounds.hpp (1)
115-117: Clear about our boundaries, we must be!Inclusive bounds with tolerance, this implementation uses. Document this choice explicitly, we should, for future Padawans to understand.
Add clarifying comment:
bool insideRadialBounds(double R, double tolerance = 0.) const final { + // Inclusive bounds check: R is inside if it falls within [minR-tol, maxR+tol] return ((R + tolerance) > get(eMinR) && (R - tolerance) < get(eMaxR)); }Core/src/Geometry/CylinderVolumeBounds.cpp (2)
255-255: Elegant simplification of values(), achieved you have!More efficient and cleaner, the new implementation is. Direct range construction, it uses, avoiding unnecessary temporary objects, it does.
Consider marking as [[nodiscard]], you should:
-std::vector<double> CylinderVolumeBounds::values() const { +[[nodiscard]] std::vector<double> CylinderVolumeBounds::values() const {
Line range hint
282-290: Enhanced validation of angles, you have implemented!More robust checks for bevel angles using radian_sym, added you have. But hmm, more specific error messages for invalid bevel angles, beneficial they would be.
Improve error messages with actual values, you should:
- "CylinderBounds: invalid bevel at min Z."); + "CylinderBounds: invalid bevel at min Z (" + std::to_string(get(eBevelMinZ)) + ")"); - "CylinderBounds: invalid bevel at max Z."); + "CylinderBounds: invalid bevel at max Z (" + std::to_string(get(eBevelMaxZ)) + ")");Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (2)
Line range hint
29-62: Documentation improvements needed, young padawan.Missing documentation for class members and methods, I see. Clear documentation, essential for the path to enlightenment, it is.
Add documentation for:
m_valuesmember variablem_ymaxmember variable- Individual enum values in
BoundValues
99-114: To the implementation file, complex calculations should move.Complex calculations in header files, increase compilation time they do. To the .cpp file, move these implementations you should:
halfPhiSector()halfLengthY()// In header double halfPhiSector() const; double halfLengthY() const; // Move implementations to .cpp fileCore/src/Surfaces/DiscTrapezoidBounds.cpp (1)
37-47: More descriptive error messages, provide we must.Include actual values in error messages, help debugging they would.
- throw std::invalid_argument("DiscTrapezoidBounds: invalid radial setup."); + throw std::invalid_argument( + fmt::format("DiscTrapezoidBounds: invalid radial setup. minR={}, maxR={}", + get(eMinR), get(eMaxR)));Core/src/Surfaces/DiscSurface.cpp (2)
90-107: Performance optimization, possible it is!In
globalToLocal, temporary Vector3 allocation occurs. For better performance, consider computing directly.- Vector3 loc3Dframe = (transform(gctx).inverse()) * position; + const auto& loc3Dframe = (transform(gctx).inverse()) * position;
Line range hint
449-488: Error messages, improved they could be!In the merging validation section, error messages could be more descriptive about the actual values that caused the failure.
- throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), - "DiscSurface::merge: surfaces are not touching in r"); + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + fmt::format("DiscSurface::merge: surfaces are not touching in r. Gap: {}", + std::min(std::abs(minR - otherMaxR), std::abs(maxR - otherMinR))));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp(0 hunks)Core/include/Acts/Surfaces/AnnulusBounds.hpp(3 hunks)Core/include/Acts/Surfaces/ConeBounds.hpp(3 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp(2 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp(0 hunks)Core/include/Acts/Surfaces/CylinderBounds.hpp(3 hunks)Core/include/Acts/Surfaces/DiamondBounds.hpp(1 hunks)Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp(3 hunks)Core/include/Acts/Surfaces/EllipseBounds.hpp(2 hunks)Core/include/Acts/Surfaces/InfiniteBounds.hpp(1 hunks)Core/include/Acts/Surfaces/LineBounds.hpp(1 hunks)Core/include/Acts/Surfaces/RadialBounds.hpp(2 hunks)Core/include/Acts/Surfaces/TrapezoidBounds.hpp(2 hunks)Core/src/Geometry/ConeVolumeBounds.cpp(2 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(1 hunks)Core/src/Geometry/CutoutCylinderVolumeBounds.cpp(5 hunks)Core/src/Geometry/CylinderVolumeBounds.cpp(2 hunks)Core/src/Geometry/TrapezoidVolumeBounds.cpp(1 hunks)Core/src/Surfaces/AnnulusBounds.cpp(8 hunks)Core/src/Surfaces/ConeBounds.cpp(3 hunks)Core/src/Surfaces/ConvexPolygonBounds.cpp(3 hunks)Core/src/Surfaces/CurvilinearSurface.cpp(1 hunks)Core/src/Surfaces/CylinderBounds.cpp(7 hunks)Core/src/Surfaces/CylinderSurface.cpp(14 hunks)Core/src/Surfaces/DiamondBounds.cpp(4 hunks)Core/src/Surfaces/DiscSurface.cpp(12 hunks)Core/src/Surfaces/DiscTrapezoidBounds.cpp(5 hunks)Core/src/Surfaces/EllipseBounds.cpp(3 hunks)Core/src/Surfaces/LineBounds.cpp(3 hunks)Core/src/Surfaces/PlaneSurface.cpp(3 hunks)Core/src/Surfaces/RadialBounds.cpp(2 hunks)Core/src/Surfaces/StrawSurface.cpp(3 hunks)Core/src/Surfaces/TrapezoidBounds.cpp(5 hunks)
💤 Files with no reviewable changes (2)
- Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp
- Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp
🚧 Files skipped from review as they are similar to previous changes (11)
- Core/include/Acts/Surfaces/InfiniteBounds.hpp
- Core/src/Surfaces/CurvilinearSurface.cpp
- Core/include/Acts/Surfaces/CylinderBounds.hpp
- Core/src/Surfaces/ConvexPolygonBounds.cpp
- Core/src/Surfaces/DiamondBounds.cpp
- Core/src/Surfaces/StrawSurface.cpp
- Core/src/Surfaces/RadialBounds.cpp
- Core/src/Surfaces/ConeBounds.cpp
- Core/src/Surfaces/TrapezoidBounds.cpp
- Core/src/Surfaces/AnnulusBounds.cpp
- Core/src/Surfaces/PlaneSurface.cpp
👮 Files not reviewed due to content moderation or server errors (2)
- Core/src/Surfaces/CylinderBounds.cpp
- Core/src/Surfaces/CylinderSurface.cpp
🔇 Additional comments (19)
Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp (2)
98-98: Approve this change, I do! Elegant and efficient, the implementation is.
Inline implementation of type(), wisdom shows. Simplified the code structure has become, yet its purpose remains clear as the waters of Naboo.
149-149: Hmmmm... Balance in the Force, I sense. Good this change is.
Mirror its counterpart above, this implementation does. Consistency in our code, maintain we must.
Core/src/Surfaces/LineBounds.cpp (1)
19-21: Clean and efficient, this implementation is!
A straightforward conversion from array to vector, providing clear access to internal values, hmm.
Core/include/Acts/Surfaces/LineBounds.hpp (1)
Line range hint 31-42: Well-crafted constructors, these are!
Parameter validation and documentation, strong with these constructors they are. Modern C++ practices, they follow.
Core/src/Surfaces/EllipseBounds.cpp (1)
21-25: Clean and clear, the namespace usage is!
Wise decision to use using declarations for frequently used helper functions, hmm. Improves readability without polluting the global namespace, it does.
Core/include/Acts/Surfaces/ConeBounds.hpp (2)
74-74: Inline implementation, approve I do!
Simple and efficient, this change is. Performance benefits, it may bring.
100-103: Wise decision to inline these methods, it is!
Simple calculations these are, inlining them improves performance it does. Clean and clear, the implementation is.
Core/include/Acts/Surfaces/RadialBounds.hpp (1)
91-93: Simple comparison with pi, correct it is!
Clean implementation of azimuth coverage check, this is. Use of std::numbers::pi, modern C++ practices it follows.
Core/include/Acts/Surfaces/TrapezoidBounds.hpp (1)
54-54: Inline type method, approve I do!
Simple and consistent with other bounds classes, this implementation is. Clean code practices, it follows.
Core/src/Geometry/CuboidVolumeBounds.cpp (1)
33-35: Elegant and efficient, this implementation is!
Using modern C++ range construction for vector initialization, wisdom you have shown. Clean and performant, the code remains.
Core/src/Geometry/CutoutCylinderVolumeBounds.cpp (1)
31-33: Consistency in the Force, I sense!
Like its cuboid cousin, this implementation follows the same pattern. Harmony in the codebase, this brings.
Core/src/Geometry/TrapezoidVolumeBounds.cpp (1)
57-59: Clean and efficient, this implementation is!
Using the range constructor for vector creation, a wise choice you have made. Performance benefits, it brings.
Core/src/Geometry/ConeVolumeBounds.cpp (1)
95-97: Consistent with TrapezoidVolumeBounds, this implementation is!
The Force is strong with this one. Range constructor, it uses wisely. Consistency across classes, it maintains.
Core/src/Geometry/CylinderVolumeBounds.cpp (2)
22-22: Hmm, wise addition of periodic utilities, this is!
For symmetry validation of angles, the new header brings essential functionality, it does.
Line range hint 282-290: Verify the usage of radian_sym across the codebase, we must!
Ensure consistent usage of angle validation in similar geometry classes, we should.
✅ Verification successful
The previous pattern for ast-grep might have been too specific. Let's try a broader search to understand the usage of angle validations and similar geometry classes. Using ripgrep will give us more context around the usage of these functions, and searching in files with "Bounds" in their name will help us find similar geometry classes, hmm.
Consistent usage of radian_sym across geometry classes, I see
Through the Force, clear it has become that radian_sym is used consistently for angle validation across all geometry classes:
- CylinderVolumeBounds: Correctly validates eAveragePhi, eBevelMinZ, and eBevelMaxZ
- RadialBounds: Validates eAveragePhi
- EllipseBounds: Validates eAveragePhi
- DiscTrapezoidBounds: Validates eAveragePhi
- CylinderBounds: Validates eAveragePhi and bevel angles
- ConeBounds: Validates eAveragePhi
- AnnulusBounds: Validates eMinPhiRel, eMaxPhiRel, and eAveragePhi
Consistent pattern of validation across the codebase, there is. No issues with the implementation, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other geometry classes using radian_sym
ast-grep --pattern 'detail::radian_sym($_)'
# Search for other implementations of values() method
ast-grep --pattern 'values() const {
return {$_};
}'
Length of output: 102
Script:
#!/bin/bash
# Search for radian_sym function usage with ripgrep for better context
rg "radian_sym" -A 2 -B 2
# Search for files containing "Bounds" in their name to find similar geometry classes
fd "Bounds" --type f --exec rg "radian|angle|phi" -A 2 -B 2 {}
Length of output: 66028
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1)
Line range hint 142-158: Wise error handling, I sense.
Private consistency checks and clear error messages, good practices they are. Strong with the Force, this implementation is.
Core/src/Surfaces/DiscSurface.cpp (3)
41-77: Constructors, well implemented they are!
Clean and efficient implementation of constructors, I see. Proper move semantics with std::move for shared pointers, used they are. Assertion for nullptr bounds, wisely added it is.
78-85:
Assignment operator, potential memory leak it has!
In the assignment operator, check for self-assignment done correctly it is. But before assigning new bounds, release old resources you should.
Apply this change, you must:
DiscSurface& DiscSurface::operator=(const DiscSurface& other) {
if (this != &other) {
Surface::operator=(other);
+ m_bounds.reset(); // Release old bounds first
m_bounds = other.m_bounds;
}
return *this;
}Likely invalid or redundant comment.
Line range hint 372-546: Complex merging logic, validation needed it is!
In the mergedWith method, extensive validation of surface properties performed is. But edge cases for numerical stability, handled they must be.
Consider extracting common validation logic into separate helper methods, clarity it brings.
✅ Verification successful
Robust and well-tested, the merging logic appears to be, hmmmm!
Examined the code thoroughly, I have. The following observations, share with you I must:
- Numerical stability well-handled it is, through consistent use of
s_onSurfaceToleranceacross all surface merging operations - Helper method
detail::mergedPhiSectorshared between disc and cylinder surfaces it is, promoting code reuse and consistency - Test coverage comprehensive it appears:
DiscSurfaceTests.cppCylinderSurfaceTests.cpp- Portal-related tests that exercise merging functionality
The validation logic in mergedWith:
- Relative rotation checks
- Translation checks
- Boundary condition validations
- Phi sector handling
All edge cases properly handled they are, with appropriate exceptions thrown when invalid conditions detected. No changes needed, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other merge implementations to verify consistency
ast-grep --pattern 'mergedWith($$$) {
$$$
throw SurfaceMergingException($$$)
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Search for all surface merging implementations and exceptions
rg -A 5 "class.*Surface" --type cpp
rg "SurfaceMergingException" --type cpp
Length of output: 66709
Script:
#!/bin/bash
# Look for numerical tolerance definitions and usage
rg "s_onSurfaceTolerance" --type cpp
rg "tolerance" -C 2 "Core/src/Surfaces/DiscSurface.cpp"
Length of output: 17965
Script:
#!/bin/bash
# Search for helper methods related to merging
rg "detail::.*merge" --type cpp
rg "detail::radian_sym" --type cpp
Length of output: 5063
Script:
#!/bin/bash
# Check for test coverage of edge cases
fd ".*Test.*cpp" --exec rg -l "mergedWith|SurfaceMergingException" {}
Length of output: 336
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Core/src/Surfaces/AnnulusBounds.cpp (3)
123-136: More descriptive, our error messages should be!Good validation you have, but clearer messages we need. Help the young padawans debug their code, we must.
Improve the error messages, you should:
if (get(eMinR) < 0. || get(eMaxR) < 0. || get(eMinR) > get(eMaxR) || std::abs(get(eMinR) - get(eMaxR)) < s_epsilon) { - throw std::invalid_argument("AnnulusBounds: invalid radial setup."); + throw std::invalid_argument( + fmt::format("AnnulusBounds: invalid radial setup. minR={}, maxR={}, epsilon={}", + get(eMinR), get(eMaxR), s_epsilon)); }
Line range hint
145-176: Document the geometry calculations, we must!Complex calculations these are. Future Jedi will thank us for clear documentation.
Add documentation to explain the geometry:
+/** + * @brief Generates vertices for the annulus bounds + * @param quarterSegments Number of segments to approximate a quarter circle + * @return Vector of 2D points representing the vertices + * + * The method generates vertices in the following order: + * 1. Inner bow vertices (reversed) + * 2. Outer bow vertices + * All vertices are transformed to the module coordinate system + */ std::vector<Vector2> AnnulusBounds::vertices( unsigned int quarterSegments) const {
Line range hint
391-402: Modernize the stream formatting, we shall!More maintainable, using fmt::format would be. Easier to read and modify, the code becomes.
Modernize the formatting:
std::ostream& AnnulusBounds::toStream(std::ostream& sl) const { - sl << std::setiosflags(std::ios::fixed); - sl << std::setprecision(7); - sl << "Acts::AnnulusBounds: (innerRadius, outerRadius, minPhi, maxPhi) = "; - sl << "(" << get(eMinR) << ", " << get(eMaxR) << ", " << phiMin() << ", " - << phiMax() << ")" << '\n'; + sl << fmt::format("Acts::AnnulusBounds: (innerRadius={:.7f}, outerRadius={:.7f}, " + "minPhi={:.7f}, maxPhi={:.7f})\n", + get(eMinR), get(eMaxR), phiMin(), phiMax());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Surfaces/AnnulusBounds.hpp(3 hunks)Core/src/Surfaces/AnnulusBounds.cpp(7 hunks)
🔇 Additional comments (3)
Core/include/Acts/Surfaces/AnnulusBounds.hpp (2)
115-117: Approve this implementation, I do!
Simple yet effective, the radial bounds checking is. Clear in its purpose, the code remains.
108-111: 🛠️ Refactor suggestion
More precise, our numerical comparison should be!
In the ways of floating-point arithmetic, careful we must be. For comparing with π, relative tolerance better suited it is.
A more robust implementation, I suggest:
bool coversFullAzimuth() const final {
- return (std::abs((get(eMinPhiRel) - get(eMaxPhiRel)) - std::numbers::pi) <
- s_onSurfaceTolerance);
+ const double phiDiff = std::abs(get(eMinPhiRel) - get(eMaxPhiRel));
+ const double relativeDiff = std::abs(phiDiff - std::numbers::pi) / std::numbers::pi;
+ return relativeDiff < 1e-10; // Relative tolerance for floating-point comparison
}Likely invalid or redundant comment.
Core/src/Surfaces/AnnulusBounds.cpp (1)
25-43: Wise decision, the anonymous namespace is!
Hidden from external sight, these helper functions are. Good practice in C++, this represents.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Core/include/Acts/Surfaces/AnnulusBounds.hpp (1)
106-109:⚠️ Potential issueMore precise, our floating-point comparison must be!
In the ways of floating-point arithmetic, careful we must be. For comparing with π, relative tolerance better suited it is.
bool coversFullAzimuth() const final { - return (std::abs((get(eMinPhiRel) - get(eMaxPhiRel)) - std::numbers::pi) < - s_onSurfaceTolerance); + const double phiDiff = std::abs(get(eMinPhiRel) - get(eMaxPhiRel)); + const double relativeDiff = std::abs(phiDiff - std::numbers::pi) / std::numbers::pi; + return relativeDiff < 1e-10; // Relative tolerance for floating-point comparison }Core/src/Surfaces/EllipseBounds.cpp (1)
66-67:⚠️ Potential issueDangerous division by zero, lurking still it is!
A previous warning, unheeded remains. When tolerance equals radius, crash your code will.
Apply this fix, you must:
- (1 < (square(lposition[eBoundLoc0] / (get(eInnerRx) - tol0)) + - square(lposition[eBoundLoc1] / (get(eOuterRx) - tol0)))); + (get(eInnerRx) <= tol0 || get(eOuterRx) <= tol0 || + 1 < (square(lposition[eBoundLoc0] / std::max(get(eInnerRx) - tol0, 1e-10)) + + square(lposition[eBoundLoc1] / std::max(get(eOuterRx) - tol0, 1e-10))));
🧹 Nitpick comments (4)
Core/include/Acts/Surfaces/AnnulusBounds.hpp (2)
113-115: Validate the tolerance parameter, we should!Negative tolerance, lead to unexpected results it may. Add validation, we must.
bool insideRadialBounds(double R, double tolerance = 0.) const final { + if (tolerance < 0.) { + throw std::invalid_argument("Negative tolerance in radial bounds check!"); + } return ((R + tolerance) > get(eMinR) && (R - tolerance) < get(eMaxR)); }
152-155: Document the return values' units, we should!Clear documentation, essential it is. Help future Padawans understand the code, it will.
- /// This method returns inner radius + /// @brief Returns the inner radius of the annulus + /// @return The inner radius in millimeters double rMin() const final { return get(eMinR); } - /// This method returns outer radius + /// @brief Returns the outer radius of the annulus + /// @return The outer radius in millimeters double rMax() const final { return get(eMaxR); }Core/src/Surfaces/EllipseBounds.cpp (1)
31-45: Strong validation, this method provides!Yet clearer error messages, we could have. More specific about the actual values that caused the error, they should be.
Like this, the error messages could be:
- throw std::invalid_argument("EllipseBounds: invalid along x axis"); + throw std::invalid_argument("EllipseBounds: invalid along x axis - inner radius (" + + std::to_string(get(eInnerRx)) + ") must be less than outer radius (" + + std::to_string(get(eOuterRx)) + ") and both must be positive");Core/include/Acts/Surfaces/EllipseBounds.hpp (1)
80-81: More specific about limitations, the warning should be!Clarify the consequences and alternatives, we must.
Like this, the warning could be:
- /// @warning This **only** works for tolerance-based checks + /// @warning This **only** works for tolerance-based checks. Will throw + /// std::logic_error for other boundary check types. Use BoundaryTolerance::asAbsoluteBoundOpt() + /// to ensure compatible tolerance type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Core/include/Acts/Surfaces/AnnulusBounds.hpp(3 hunks)Core/include/Acts/Surfaces/EllipseBounds.hpp(2 hunks)Core/include/Acts/Surfaces/LineBounds.hpp(1 hunks)Core/src/Surfaces/AnnulusBounds.cpp(7 hunks)Core/src/Surfaces/EllipseBounds.cpp(3 hunks)Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp
- Core/include/Acts/Surfaces/LineBounds.hpp
- Core/src/Surfaces/AnnulusBounds.cpp
🔇 Additional comments (6)
Core/include/Acts/Surfaces/AnnulusBounds.hpp (2)
71-71: Approve this change, I do!
Simple and efficient, this inline implementation is. The way of the Force, it follows.
118-121: Approve these binning methods, I do!
Simple and efficient, these implementations are. The Force flows through them smoothly.
Core/src/Surfaces/EllipseBounds.cpp (3)
14-25: Wise changes in namespace usage and includes, I sense!
Clean and organized, the code becomes. Using directives for perp and phi, readability they improve.
27-29: Simple yet effective, this implementation is!
Clean access to internal values, it provides. The Force is strong with this one.
77-82: Clean and efficient, this implementation is!
Helper function well used, it is. The way of the Jedi, this follows.
Core/include/Acts/Surfaces/EllipseBounds.hpp (1)
69-69: Simple and direct, this implementation now is!
Unnecessary complexity, removed it has been. The Force flows better now.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Core/src/Surfaces/ConeSurface.cpp (1)
Line range hint
271-280: Hmm, careful with tiny numbers, we must be!A larger epsilon value, safer it would be. Numerical stability, we must ensure.
Consider this change, you should:
- if (A == 0.) { - A += 1e-16; // avoid division by zero + if (std::abs(A) < 1e-10) { // Use more stable threshold + A = std::copysign(1e-10, A); // Preserve sign
♻️ Duplicate comments (1)
Core/src/Surfaces/EllipseBounds.cpp (1)
63-67:⚠️ Potential issueBeware of division by zero, you must!
A disturbance in the Force, I sense! When tolerance equals radius, divide by zero we might. Previous warning, unheeded remains!
Apply this fix, you should:
- bool insideInner = (get(eInnerRx) <= tol0) || (get(eOuterRx) <= tol0) || - (1 < (square(lposition[0] / (get(eInnerRx) - tol0)) + - square(lposition[1] / (get(eOuterRx) - tol0)))); - bool insideOuter = (square(lposition[0] / (get(eInnerRy) + tol0)) + - square(lposition[1] / (get(eOuterRy) + tol0))) < 1; + bool insideInner = (get(eInnerRx) <= tol0) || (get(eOuterRx) <= tol0) || + (1 < (square(lposition[0] / std::max(get(eInnerRx) - tol0, 1e-10)) + + square(lposition[1] / std::max(get(eOuterRx) - tol0, 1e-10)))); + bool insideOuter = (square(lposition[0] / std::max(get(eInnerRy) + tol0, 1e-10)) + + square(lposition[1] / std::max(get(eOuterRy) + tol0, 1e-10))) < 1;
🧹 Nitpick comments (24)
Core/src/Surfaces/EllipseBounds.cpp (2)
26-28: Consider const reference return, you should!For large vectors, more efficient it would be, to return by const reference. Like this, it should look:
-std::vector<double> EllipseBounds::values() const { - return {m_values.begin(), m_values.end()}; +const std::vector<double>& EllipseBounds::values() const { + return m_values;
Line range hint
85-95: Twice printed, the averagePhi value is!In the stream output, repeated the averagePhi value becomes. Fix this small oversight, we must!
- sl << "(" << get(eInnerRx) << ", " << get(eInnerRy) << ", " << get(eOuterRx) - << ", " << get(eOuterRy) << ", " << get(eAveragePhi) << ", " - << get(eHalfPhiSector) << ", " << get(eAveragePhi) << ")"; + sl << "(" << get(eInnerRx) << ", " << get(eInnerRy) << ", " << get(eOuterRx) + << ", " << get(eOuterRy) << ", " << get(eHalfPhiSector) << ", " + << get(eAveragePhi) << ")";Core/src/Surfaces/LineBounds.cpp (2)
19-21: Optimize the Force within this method, we must!Return by const reference, we should, when performance matters most.
-std::vector<double> LineBounds::values() const { +const std::vector<double>& LineBounds::values() const { - return {m_values.begin(), m_values.end()}; + return m_values; }
Line range hint
41-49: Wisdom in streaming, we have found, but optimize further, we can!Use string_view for constant string literals, save memory we will.
std::ostream& LineBounds::toStream(std::ostream& sl) const { sl << std::setiosflags(std::ios::fixed); sl << std::setprecision(7); - sl << "Acts::LineBounds: (radius, halflengthInZ) = "; + sl << std::string_view("Acts::LineBounds: (radius, halflengthInZ) = "); sl << "(" << get(LineBounds::eR) << ", " << get(LineBounds::eHalfLengthZ) << ")"; sl << std::setprecision(-1); return sl; }Core/src/Surfaces/ConeSurface.cpp (1)
Line range hint
15-18: Misaligned, these else clauses are!Fix the indentation, we must. Consistency in style, important it is.
Apply this correction:
if loyalty_years >= 5: discount = 0.15 - elif loyalty_years >= 3: - discount = 0.1 - else: - discount = 0 + elif loyalty_years >= 3: + discount = 0.1 + else: + discount = 0Core/src/Surfaces/DiscSurface.cpp (2)
Line range hint
369-543: Extract constants, we should! Clearer code, it brings!Long function this is, but well-structured it remains. Yet, magic numbers like tolerance, extract we should.
Consider this improvement:
+ // At the top of the file, with other constants + constexpr double MERGE_TOLERANCE = s_onSurfaceTolerance; + constexpr double MIN_ROTATION_TOLERANCE = 1e-10; - constexpr auto tolerance = s_onSurfaceTolerance; + constexpr auto tolerance = MERGE_TOLERANCE;
194-195: Optimize this function, we can! Performance gains, it brings!Small vector operations, faster they can be. Consider SIMD instructions, we should.
Consider this optimization:
-Vector2 DiscSurface::localPolarToCartesian(const Vector2& lpolar) const { - return Vector2(lpolar[0] * cos(lpolar[1]), lpolar[0] * sin(lpolar[1])); +Vector2 DiscSurface::localPolarToCartesian(const Vector2& lpolar) const { + double sincos[2]; + sincos(lpolar[1], &sincos[0], &sincos[1]); // Compute sin and cos together + return Vector2(lpolar[0] * sincos[1], lpolar[0] * sincos[0]);Core/src/Surfaces/LineSurface.cpp (2)
Line range hint
58-95: Documentation for geometric calculations, improve we must!Complex geometric transformations, these methods contain. Additional comments explaining the mathematical operations, helpful they would be.
Consider adding explanatory comments for the geometric calculations:
Vector3 LineSurface::localToGlobal(const GeometryContext& gctx, const Vector2& lposition, const Vector3& direction) const { + // Get the line direction vector (local z-axis) Vector3 unitZ0 = lineDirection(gctx); + // Calculate the radial direction perpendicular to both line and momentum Vector3 radiusAxisGlobal = unitZ0.cross(direction);
Line range hint
140-184: Complex intersection logic, break down we should!Hmm, correct the implementation is, but clarity it could gain. Extract helper methods for boundary checks and intersection calculations, we should.
Consider refactoring the intersection calculation:
+ // Helper method to calculate intersection parameters + struct IntersectionParams { + double u; + Vector3 result; + IntersectionStatus status; + }; + + IntersectionParams calculateIntersection(const Vector3& ma, const Vector3& ea, + const Vector3& mb, const Vector3& eb, + double tolerance) { + Vector3 mab = mb - ma; + double eaTeb = ea.dot(eb); + double denom = 1 - eaTeb * eaTeb; + + if (std::abs(denom) < std::abs(tolerance)) { + return {0, Vector3::Zero(), IntersectionStatus::invalid}; + } + + double u = (mab.dot(ea) - mab.dot(eb) * eaTeb) / denom; + IntersectionStatus status = std::abs(u) > std::abs(tolerance) + ? IntersectionStatus::reachable + : IntersectionStatus::onSurface; + return {u, ma + u * ea, status}; + }Core/src/Surfaces/Surface.cpp (4)
Line range hint
50-124: Document the ways of the Force, we must!Complex mathematical operations, these methods contain. Yet documentation explaining the alignment calculations, sparse it remains. Help future Padawans understand this code better, we should.
Consider adding detailed comments explaining:
- The mathematical concepts behind alignment calculations
- The meaning of each matrix operation
- The coordinate system transformations
Line range hint
159-204: Strong with equality comparisons, this code is, but optimize it we can!Wise implementation of operators I see, but improve performance we might. Consider reordering equality checks from least to most expensive:
- Pointer comparison (already first, good!)
- Type comparison (already second, good!)
- Move transform comparison before bounds comparison, as matrix comparison might be faster than bounds comparison in many cases.
if (&other == this) { return true; } if (other.type() != type()) { return false; } + // Compare transform values first (usually faster) + if (m_transform && other.m_transform && + !m_transform->isApprox((*other.m_transform), 1e-9)) { + return false; + } if (other.bounds() != bounds()) { return false; } if (m_associatedDetElement != other.m_associatedDetElement) { return false; } - if (m_transform && other.m_transform && - !m_transform->isApprox((*other.m_transform), 1e-9)) { - return false; - }
214-217: Temporary objects, create unnecessarily we do!Optimize memory usage and improve const correctness, we can. The Force guides us to better performance!
- RotationMatrix3 rot(transform(gctx).matrix().block<3, 3>(0, 0)); - Vector3 rotX(rot.col(0)); - Vector3 rotY(rot.col(1)); - Vector3 rotZ(rot.col(2)); + const auto& rot = transform(gctx).matrix().block<3, 3>(0, 0); + const auto& rotX = rot.col(0); + const auto& rotY = rot.col(1); + const auto& rotZ = rot.col(2);
337-349: Validate the path to the Dark Side leads, we must!Though resource management strong it is, additional validation add we should. Protect against null pointers and invalid states, we must.
void Surface::assignDetectorElement(const DetectorElementBase& detelement) { + // Ensure we're not already associated with a different detector element + if (m_associatedDetElement != nullptr && m_associatedDetElement != &detelement) { + throw std::runtime_error("Surface already associated with a different detector element"); + } m_associatedDetElement = &detelement; m_transform.reset(); } void Surface::assignSurfaceMaterial( std::shared_ptr<const ISurfaceMaterial> material) { + // Validate material before assignment + if (material && !material->isValid()) { + throw std::invalid_argument("Invalid surface material"); + } m_surfaceMaterial = std::move(material); }Core/src/Surfaces/DiscTrapezoidBounds.cpp (6)
22-30: Document the parameters' constraints, we should!Valid ranges for the parameters, documented they must be. Help future Padawans understand the bounds' requirements, this will.
Add documentation like this, you should:
/** * @param halfXminR Half-length in x at minimum radius (must be positive) * @param halfXmaxR Half-length in x at maximum radius (must be positive) * @param minR Minimum radius (must be positive) * @param maxR Maximum radius (must be greater than minR) * @param avgPhi Average phi angle * @param stereo Stereo angle * @throws std::invalid_argument if parameters violate constraints */
32-34: Return by const reference, we could!Performance benefits, returning a const reference to m_values would bring. Unless copying is required by design, this change consider you should.
const std::vector<double>& values() const { return m_values; }
36-46: More specific in our error messages, we must be!Help developers diagnose issues faster, detailed error messages will. Include the actual values in the error messages, we should.
- throw std::invalid_argument("DiscTrapezoidBounds: invalid radial setup."); + throw std::invalid_argument("DiscTrapezoidBounds: invalid radial setup. " + "Required: minR >= 0 && maxR > 0 && minR <= maxR. " + "Got: minR=" + std::to_string(get(eMinR)) + + ", maxR=" + std::to_string(get(eMaxR)));
Line range hint
64-73: More descriptive names for vertices, use we must!Clearer intent, better variable names would show. Understand the geometry better, future maintainers will.
- Vector2 vertices[] = {{get(eHalfLengthXminR), get(eMinR)}, + Vector2 trapezoidCorners[] = {{get(eHalfLengthXminR), get(eMinR)}, // top right
Line range hint
76-87: Document the vertex order, we must!Clear understanding of vertex ordering, crucial it is for polygon operations. Add documentation about clockwise/counterclockwise ordering, we should.
Add documentation like this:
/** * @brief Returns vertices in counter-clockwise order starting from bottom-left * @param ignoredSegments Unused parameter (consider removing) * @return Vector of vertices defining the trapezoid boundary */
Line range hint
89-102: Use string_view for constant strings, we should!More efficient, string_view would be for constant string literals. Modern C++ practices, embrace we must.
- sl << "Acts::DiscTrapezoidBounds: (innerRadius, outerRadius, " - "halfLengthXminR, " + sl << std::string_view{"Acts::DiscTrapezoidBounds: (innerRadius, outerRadius, " + "halfLengthXminR, "};Core/src/Surfaces/RectangleBounds.cpp (5)
19-33: Hmmmm, wise implementation this is, but room for improvement, there still may be.Clean and efficient, your switch statement is. Handle invalid cases with assertion and NaN, you do. Yet consider, you should, marking the function as [[nodiscard]] to ensure its return value, ignored it is not.
-double RectangleBounds::get(BoundValues bValue) const { +[[nodiscard]] double RectangleBounds::get(BoundValues bValue) const {
35-42: Strong with the Force, this validation is. Documentation, it needs.Validate the bounds correctly, you do. But document the exceptions thrown, you should, so that prepared, other Jedi may be.
+/** + * @throws std::invalid_argument if minimum values exceed maximum values + */ void RectangleBounds::checkConsistency() noexcept(false) {
50-53: Clear as the waters of Dagobah, this implementation is.Counter-clockwise order, you maintain. Yet unused parameter /lseg/, explain its purpose in comments, you should.
-std::vector<Vector2> RectangleBounds::vertices(unsigned int /*lseg*/) const { +// lseg parameter kept for interface compatibility but not used for rectangles +std::vector<Vector2> RectangleBounds::vertices(unsigned int /*lseg*/) const {
55-57: Simple yet profound, like a Jedi's wisdom, this method is.Returns itself, a rectangle does, for it is its own bounding box. Document this wisdom, we should.
+/// @return Reference to self as a rectangle is its own bounding box const RectangleBounds& RectangleBounds::boundingBox() const {
Line range hint
59-68: Wisdom in streams, you show. Yet efficiency, we can improve.Format with precision, you do well. But pass std::ios::fixed as const reference, more efficient it would be.
- sl << std::setiosflags(std::ios::fixed); + sl << std::setiosflags(static_cast<std::ios_base::fmtflags>(std::ios::fixed));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp(3 hunks)Core/src/Surfaces/AnnulusBounds.cpp(11 hunks)Core/src/Surfaces/ConeBounds.cpp(2 hunks)Core/src/Surfaces/ConeSurface.cpp(10 hunks)Core/src/Surfaces/CylinderBounds.cpp(7 hunks)Core/src/Surfaces/CylinderSurface.cpp(14 hunks)Core/src/Surfaces/DiscSurface.cpp(11 hunks)Core/src/Surfaces/DiscTrapezoidBounds.cpp(4 hunks)Core/src/Surfaces/EllipseBounds.cpp(3 hunks)Core/src/Surfaces/LineBounds.cpp(2 hunks)Core/src/Surfaces/LineSurface.cpp(8 hunks)Core/src/Surfaces/PlaneSurface.cpp(3 hunks)Core/src/Surfaces/RadialBounds.cpp(2 hunks)Core/src/Surfaces/RectangleBounds.cpp(2 hunks)Core/src/Surfaces/Surface.cpp(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Core/src/Surfaces/ConeBounds.cpp
- Core/src/Surfaces/RadialBounds.cpp
- Core/src/Surfaces/CylinderBounds.cpp
- Core/src/Surfaces/AnnulusBounds.cpp
- Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp
- Core/src/Surfaces/PlaneSurface.cpp
- Core/src/Surfaces/CylinderSurface.cpp
🔇 Additional comments (17)
Core/src/Surfaces/EllipseBounds.cpp (2)
13-24: Approve these changes, I do!
Simplified the namespace usage has been, hmm! Cleaner and more maintainable, the code becomes.
30-44: Strong with the Force, this validation is!
Clear error messages and thorough checks, you have provided. Approve this implementation, I do!
Core/src/Surfaces/LineBounds.cpp (2)
17-18: Clean and precise, this namespace declaration is!
Simple yet effective, the way of the Force it follows.
32-39: Elegant and efficient, this boundary check is!
The Force flows smoothly through this implementation, it does.
Core/src/Surfaces/ConeSurface.cpp (1)
31-32: Wise choice, the namespace cleanup is!
Clear and concise, these using directives are. Within the Acts namespace, properly placed they are.
Core/src/Surfaces/DiscSurface.cpp (1)
29-29: Mmm, good addition this header is!
For angle calculations in mergedWith function, essential this include is.
Core/src/Surfaces/LineSurface.cpp (6)
27-42: Constructors, well implemented they are!
Proper initialization of base classes and smart pointer management, I see. Nullptr check with throw_assert, wisdom it shows.
43-56: Copy operations, properly implemented they are!
Self-assignment check and proper base class handling, present they are. The Force, strong with this code it is.
Line range hint 186-221: Well documented Jacobian calculations, I sense!
Reference to white paper and clear mathematical operations, present they are. Strong with documentation, this code is.
Line range hint 223-250: Path derivatives, well implemented they are!
Clear variable names and proper assertions, wisdom they show. Mathematical precision, strong it is.
Line range hint 252-280: Alignment derivatives, elegant they are!
Modern C++ features and clear structure, well used they are. The Force of clean code, strong with this one it is.
Line range hint 282-297: Helper methods, concise and clear they are!
Simple yet powerful, these implementations are. The path of the Jedi, they follow.
Core/src/Surfaces/Surface.cpp (3)
22-24: Clean and proper, the surface type names array now is!
Simplified namespace usage without the redundant Acts:: prefix, we have achieved. Unchanged functionality, we maintain.
Line range hint 26-47: Wise implementation of constructors, I sense!
Handle resources with care, these constructors do. Smart pointers they use, and member initialization they perform correctly. The Force is strong with this code!
236-248: Efficient access to transformations, achieved we have!
Direct matrix access for center calculation, wise choice it is. Proper const correctness and reference handling, maintained they are.
Core/src/Surfaces/DiscTrapezoidBounds.cpp (1)
13-13: Hmm, clean and organized, these changes are!
Wrapped in the Acts namespace, the entire implementation now is. A path to clarity, this brings!
Also applies to: 18-18, 20-21
Core/src/Surfaces/RectangleBounds.cpp (1)
44-48: Mysterious, this std::nullopt is. Verify its purpose, we must.
Clear, the implementation is, yet the purpose of std::nullopt, documented it should be. Search your feelings about this parameter's significance.
…some-surface-code
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/src/Surfaces/DiscTrapezoidBounds.cpp (2)
23-35: Optimize the values() method, we must!Return by const reference, we should, to avoid unnecessary copying, yes.
-std::vector<double> DiscTrapezoidBounds::values() const { - return {m_values.begin(), m_values.end()}; +const std::vector<double>& DiscTrapezoidBounds::values() const { + return m_values;
Line range hint
77-88: Improve readability with const variables, we shall!More readable and maintainable, the code will be, with const variables for intermediate calculations.
std::vector<Vector2> DiscTrapezoidBounds::vertices( unsigned int /*ignoredSegments*/) const { - Vector2 cAxis(std::cos(get(eAveragePhi)), std::sin(get(eAveragePhi))); - Vector2 nAxis(cAxis.y(), -cAxis.x()); - auto ymin = std::sqrt(get(eMinR) * get(eMinR) - + const Vector2 cAxis(std::cos(get(eAveragePhi)), std::sin(get(eAveragePhi))); + const Vector2 nAxis(cAxis.y(), -cAxis.x()); + const auto ymin = std::sqrt(get(eMinR) * get(eMinR) - get(eHalfLengthXminR) * get(eHalfLengthXminR)); - auto halfY = (m_ymax - ymin) / 2; + const auto halfY = (m_ymax - ymin) / 2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp(3 hunks)Core/src/Surfaces/AnnulusBounds.cpp(11 hunks)Core/src/Surfaces/DiscTrapezoidBounds.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/src/Surfaces/AnnulusBounds.cpp
- Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp
🔇 Additional comments (4)
Core/src/Surfaces/DiscTrapezoidBounds.cpp (4)
11-22: Organized well, the includes are!
Clean and necessary, these additions are. Wrapped in namespace Acts, the implementation now is, hmm.
37-47: Strong with the Force, this validation is!
Clear error messages and thorough checks, you have provided. Pleased, Master Yoda is!
55-63: Cache trigonometric calculations, we must!
Calculated twice, the same trigonometric values are. Optimize performance, caching these values would.
SquareMatrix2 DiscTrapezoidBounds::jacobianToLocalCartesian(
const Vector2& lposition) const {
SquareMatrix2 jacobian;
- jacobian(0, 0) = std::sin(lposition[1] - get(eAveragePhi));
- jacobian(1, 0) = std::cos(lposition[1] - get(eAveragePhi));
+ const double angleDiff = lposition[1] - get(eAveragePhi);
+ const double sinAngle = std::sin(angleDiff);
+ const double cosAngle = std::cos(angleDiff);
+ jacobian(0, 0) = sinAngle;
+ jacobian(1, 0) = cosAngle;
jacobian(0, 1) = lposition[0] * std::cos(lposition[1]);
jacobian(1, 1) = lposition[0] * -std::sin(lposition[1]);
return jacobian;Line range hint 90-104: Well formatted, this output is!
Clear and precise, the stream formatting is. All parameters, included they are.
|
Summary by CodeRabbit
New Features
values()method to multiple classes (e.g.,AnnulusBounds,ConeBounds,DiamondBounds,DiscTrapezoidBounds,EllipseBounds,RadialBounds,RectangleBounds,TrapezoidVolumeBounds) to return internal state as a vector of doubles.checkConsistency()method in several classes to validate parameters and throw exceptions for invalid configurations.Bug Fixes
checkConsistency()methods across various classes to throwstd::invalid_argumentexceptions for invalid inputs.Refactor
Documentation
Tests
Chores