feat: Adding cuboid volume stack #4040
Conversation
WalkthroughHmm, a significant evolution in the ACTS geometry module, this is! A new Changes
Sequence DiagramsequenceDiagram
participant VS as Volume Stack
participant V1 as Volume 1
participant V2 as Volume 2
participant Strategy as Attachment Strategy
VS->>Strategy: Select attachment method
Strategy->>V1: Analyze first volume
Strategy->>V2: Analyze second volume
Strategy-->>VS: Compute attachment
VS->>VS: Update volume bounds
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Core/include/Acts/Geometry/CuboidVolumeStack.hpp (1)
21-21: Improve class description for clarity, you must.The sentence on line 21, "This class implements a x-. y-. z-aligned stack," contains a typo and could be clearer. Consider rephrasing to "This class implements an x-, y-, z-aligned stack of cuboid volumes with synchronized bounds."
Core/src/Geometry/CuboidVolumeStack.cpp (3)
112-114: Address the TODO comment, we must.A TODO on line 113 questions the appropriate tolerance value. Determine a suitable tolerance, perhaps by reviewing similar usages or consulting domain requirements.
Do you wish me to suggest an appropriate tolerance value for this case?
311-313: Clarify the tolerance value, we should.Another TODO on line 311 regarding the tolerance. Consistency in tolerance values across the codebase, maintain we must. Recommend reviewing and setting an appropriate value.
May I assist in determining a consistent and suitable tolerance value here?
484-485: Resolve the TODO about rotation precision, we should.The TODO on line 485 indicates uncertainty about rotation precision. Establish the required precision for rotation comparisons to ensure accurate alignment checks.
Shall I help in suggesting an appropriate rotation precision value?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Geometry/CuboidVolumeStack.hpp(1 hunks)Core/src/Geometry/CuboidVolumeStack.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: build_debug
- GitHub Check: macos
🔇 Additional comments (3)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (2)
43-43: Configure static analysis tool for Boost macros, you should.Static analysis reports an unknown macro at line 43 (
BOOST_FIXTURE_TEST_SUITE). Ensure that the static analysis tool is configured to recognize Boost Test macros to avoid false positives.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
1040-1042: Use of empty volume vector, verify we must.In the test case starting at line 1040, you check for an empty volume vector. Confirm that throwing an
std::invalid_argumentin this scenario aligns with the intended behavior and exception handling policies.Tests/UnitTests/Core/Geometry/CMakeLists.txt (1)
35-35: Hmmmm, good this addition is!Properly placed, the new test for CuboidVolumeStack is. With its cylindrical cousin, grouped logically it stands. The Force, strong with this test organization, it is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Core/src/Geometry/CuboidVolumeStack.cpp (4)
112-113: Specify appropriate tolerance, you must.A suitable tolerance for axis alignment, determine and set you should. Precise comparisons, it will ensure.
Assist you, I can. Wish you that I suggest an appropriate value?
310-311: Resolve the TODO about tolerance, you must.To avoid incorrect overlap detection, an appropriate tolerance, specify you should.
Help in choosing a suitable tolerance value, I can. Generate a suggestion, would you like?
484-485: Define rotation precision tolerance, you should.For accurate alignment checks, specify the tolerance for rotation precision, you must.
Aid you, I can, in selecting a proper tolerance for rotation comparisons.
845-846: Consider reusingm_transform, you should.As noted in the TODO comment, reusing
m_transformmay optimize the code. Implement this improvement, you should.Apply this diff to reuse
m_transform:- // @TODO: We probably can reuse m_transform + // Reusing m_transform to optimize m_groupTransform = m_transform;docs/_extensions/lazy_autodoc.py (1)
99-99: Maintain alphabetical order, you must.Insert
"Acts::CuboidVolumeStack"after"Acts::CuboidVolumeBounds"to keep the list organized.Apply this diff to correct the order:
"Acts::ConeVolumeBounds", "Acts::CuboidVolumeBounds", + "Acts::CuboidVolumeStack", "Acts::CylinderVolumeBounds", "Acts::CutoutCylinderVolumeBounds",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/src/Geometry/CuboidVolumeStack.cpp(1 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: build_debug
🔇 Additional comments (1)
Core/src/Geometry/CuboidVolumeStack.cpp (1)
603-604: Verify the suitability ofs_onSurfaceTolerance, you should.Ensure that
s_onSurfaceToleranceprovides adequate precision for your comparisons, you must.Confirm that
s_onSurfaceToleranceis appropriate for this context. Assist you, I can, if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (4)
37-45: Document the purpose of this fixture, young padawan must.Clear documentation for the Fixture class, missing it is. Explain its purpose and the logging threshold manipulation, you should.
struct Fixture { + /// Stores the original logging level Logging::Level m_level; + + /// Constructor: Temporarily changes the logging threshold to FATAL + /// to suppress expected warning messages during tests Fixture() { m_level = Acts::Logging::getFailureThreshold(); Acts::Logging::setFailureThreshold(Acts::Logging::FATAL); } + /// Destructor: Restores the original logging threshold ~Fixture() { Acts::Logging::setFailureThreshold(m_level); } };
63-338: Refactor this test case, we must. Too complex, it has become.Break down the large test case into smaller, focused helper functions, we should. Improve readability and maintainability, this will.
Example helper functions to extract:
/// Helper to create bounds for test volumes std::shared_ptr<CuboidVolumeBounds> createTestBounds( double halfDir, double boundDirOrth1, double boundDirOrth2, CuboidVolumeBounds::BoundValues boundDir) { std::array<double, 3> boundsVals; boundsVals.at(boundDir) = halfDir; boundsVals.at(boundDirOrth1) = boundDirOrth1; boundsVals.at(boundDirOrth2) = boundDirOrth2; return std::make_shared<CuboidVolumeBounds>(boundsVals); } /// Helper to verify volume bounds void verifyVolumeBounds(const Volume* volume, double expectedOrth1, double expectedOrth2, CuboidVolumeBounds::BoundValues boundDirOrth1, CuboidVolumeBounds::BoundValues boundDirOrth2) { const auto* cuboidBounds = dynamic_cast<const CuboidVolumeBounds*>(&volume->volumeBounds()); BOOST_REQUIRE(cuboidBounds != nullptr); BOOST_CHECK_EQUAL(cuboidBounds->get(boundDirOrth1), expectedOrth1); BOOST_CHECK_EQUAL(cuboidBounds->get(boundDirOrth2), expectedOrth2); }
896-923: Improve the ASCII art diagram, we shall.Helpful the diagram is, but clearer it could be. Add annotations for dimensions and explain the transformation steps better, you should.
// original size -// <---------------> +// <-----200mm-----> // +---------------+ // | | // | | // | Volume 1 | // | | // | | // +---------------+ // first resize -// <--------------------------> +// <---------300mm----------> (+100mm) // +---------------+----------+ // | | | // | | | // | Volume 1 | Gap | // | | | Gap is // | | | reused!--+ // +---------------+----------+ | // second resize | -// <-----------------------------------> | +// <---------400mm--------------> (+100mm) | // +---------------+-------------------+ | // | | | | // | | | | // | Volume 1 | Gap |<-----+ // | | | // | | | // +---------------+-------------------+
1046-1065: Strong with error handling, this test is.Good validation of invalid directions, you have implemented. Consider adding test cases for additional edge cases, you might.
Additional test cases to consider:
BOOST_TEST_CONTEXT("Invalid axis combinations") { // Test with conflicting axis directions BOOST_CHECK_THROW( CuboidVolumeStack(volumes, static_cast<AxisDirection>(-1), strategy), std::invalid_argument); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 47-47: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / clang_tidy
There was a problem hiding this comment.
Nice! Overall implementation logic and tests seem fine to me.
My only concern is casting between the different enums, and using the enum int values directly as indices.
While this currently works, I think we should treat the fact that these enums align in this way as coincidence.
What I mean by that is that the conversion, and also indexing into e.g. Eigen::Vector3::Unit(X) should be done using the semantic labels.
So instead of something like
auto d = AxisDirection::AxisX;
auto v = Vector3d::Unit(static_cast<std::size_t>(d));I think we should do something like this
std::size_t toIndex(AxisDirection d) {
swith(d) {
case AxisX: return 0;
case AxisY: return 1;
case AxisZ: return 2;
default: throw std::invalid_argument();
}
}
auto d = AxisDirection::AxisX;
auto v = Vector3d::Unit(toIndex(d));The same can be done for converting between AxisDirection and e.g. CuboidVolumeBounds::BoundValue.
With indices / conversion done using the named labels, I don't have any other objections against using the indices.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Core/include/Acts/Geometry/CuboidVolumeStack.hpp (1)
90-92: 🛠️ Refactor suggestionAvoid unnecessary copying of shared pointers, you should.
Passing
volboundsandtransformby value may cause unnecessary copying. Better it is to pass them by const reference to improve efficiency. Modify the function signature as follows:- void update(std::shared_ptr<VolumeBounds> volbounds, - std::optional<Transform3> transform = std::nullopt, - const Logger& logger = getDummyLogger()) override; + void update(const std::shared_ptr<VolumeBounds>& volbounds, + const std::optional<Transform3>& transform = std::nullopt, + const Logger& logger = getDummyLogger()) override;
🧹 Nitpick comments (21)
Core/src/Geometry/CuboidVolumeBounds.cpp (3)
12-12: Remove unused header, hmm.Unnecessary include of
LineSurface.hppappears to be, as no usage of this header in the implementation I see.-#include "Acts/Surfaces/LineSurface.hpp"
37-53: A better initialization approach, suggest I do.Using -1 as sentinel value, risky it is. A more robust approach, consider:
- Use std::optional for uninitialized values
- Or a dedicated enum for initialization state
Example implementation:
- std::array<double, eSize> values = {-1, -1, -1}; + std::array<std::optional<double>, eSize> values = {std::nullopt, std::nullopt, std::nullopt}; for (const auto& [key, value] : keyValues) { - values[key] = value; + values[key] = std::make_optional(value); } if (std::any_of(values.begin(), values.end(), - [](const auto& val) { return val == -1; })) { + [](const auto& val) { return !val.has_value(); })) { throw std::logic_error("Missing bound values"); } - m_values = values; + m_values = {values[0].value(), values[1].value(), values[2].value()};
175-191: Remove redundant break statements, we must.After return statements, break statements serve no purpose they do. Remove them, we should.
switch (direction) { case AxisX: return BoundValues::eHalfLengthX; - break; case AxisY: return BoundValues::eHalfLengthY; - break; case AxisZ: return BoundValues::eHalfLengthZ; - break; default: throw std::invalid_argument("Invalid axis direction"); }Core/include/Acts/Geometry/CylinderVolumeStack.hpp (1)
14-15: Approve the strategy changes, I do. Documentation update, suggest I must.Well-structured changes to use new strategy types, these are. But document the new strategy types in class documentation, we should.
Add documentation about the new strategy types in the class documentation:
/// @class CylinderVolumeStack /// This class implements a z-aligned or r-aligned stack /// of cylinder volumes with synchronized bounds. +/// The stack behavior can be configured using: +/// - @c VolumeAttachmentStrategy: Controls how volumes are attached +/// - @c VolumeResizeStrategy: Controls how volumes are resizedAlso applies to: 56-57, 91-92, 125-125, 141-141, 166-166
Tests/UnitTests/Core/Geometry/CuboidVolumeBoundsTests.cpp (2)
41-45: More comprehensive tests, require we do.Good start this test is, but more cases cover we must:
- Test different order of initialization
- Test with duplicate keys
- Test with invalid values
// Test different initialization order CuboidVolumeBounds init_diff_order( {{CuboidVolumeBounds::BoundValues::eHalfLengthZ, hz}, {CuboidVolumeBounds::BoundValues::eHalfLengthX, hx}, {CuboidVolumeBounds::BoundValues::eHalfLengthY, hy}}); BOOST_CHECK_EQUAL(box, init_diff_order); // Test duplicate keys (should use last value) CuboidVolumeBounds init_duplicate( {{CuboidVolumeBounds::BoundValues::eHalfLengthX, hx}, {CuboidVolumeBounds::BoundValues::eHalfLengthX, hx * 2}, // Duplicate {CuboidVolumeBounds::BoundValues::eHalfLengthY, hy}, {CuboidVolumeBounds::BoundValues::eHalfLengthZ, hz}}); BOOST_CHECK_EQUAL(init_duplicate.get(CuboidVolumeBounds::eHalfLengthX), hx * 2);
80-84: More exception tests, write we must.Test all missing value combinations, we should. More thorough coverage, this provides.
// Test missing X BOOST_CHECK_THROW( CuboidVolumeBounds({{CuboidVolumeBounds::BoundValues::eHalfLengthY, hy}, {CuboidVolumeBounds::BoundValues::eHalfLengthZ, hz}}), std::logic_error); // Test missing Y BOOST_CHECK_THROW( CuboidVolumeBounds({{CuboidVolumeBounds::BoundValues::eHalfLengthX, hx}, {CuboidVolumeBounds::BoundValues::eHalfLengthZ, hz}}), std::logic_error); // Test missing Z BOOST_CHECK_THROW( CuboidVolumeBounds({{CuboidVolumeBounds::BoundValues::eHalfLengthX, hx}, {CuboidVolumeBounds::BoundValues::eHalfLengthY, hy}}), std::logic_error); // Test empty initializer list BOOST_CHECK_THROW( CuboidVolumeBounds({}), std::logic_error);Core/src/Geometry/CylinderContainerBlueprintNode.cpp (1)
Line range hint
210-214: Fix error message in setAttachmentStrategy, we must.Incorrect error message exists. Speaks of direction when attachment strategy it means.
if (m_stack != nullptr) { - throw std::runtime_error("Cannot change direction after build"); + throw std::runtime_error("Cannot change attachment strategy after build"); }Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp (1)
276-277: Refactored strategy usage, observe I do.From
CylinderVolumeStackspecific strategies to more generalizedVolumeAttachmentStrategyandVolumeResizeStrategy, the transition is made. Support forCuboidVolumeStackand future volume types, this change does enable.Consider documenting the strategy selection guidelines in the class documentation, help future developers it will.
Also applies to: 281-281, 312-313, 346-346
Examples/Python/src/Blueprint.cpp (1)
344-347: Updated constructor parameters, I observe.From
CylinderVolumeStackspecific types to generalized strategy types, the transition is made. Default values preserved, backward compatibility maintained.Consider adding Python docstrings to explain the strategy options, guide the Python developers it will.
Examples/Python/src/Geometry.cpp (1)
299-307: Python enum bindings, updated they are.From
CylinderVolumeStackspecific enums to generalized strategy enums, the transition is made. Same values preserved, backward compatibility maintained.Consider adding enum value descriptions in Python docstrings, help Python developers understand strategy options it will.
Core/src/Geometry/CuboidVolumeStack.cpp (7)
146-154: More flexible, the tolerance should be!Use the standard surface tolerance constant, you should. Hard-coded values, lead to maintenance difficulties they do.
Apply this diff to use standard tolerance:
- if ((localDirVector - Vector3::UnitX()).norm() < 1e-4) { + if ((localDirVector - Vector3::UnitX()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisX; - } else if ((localDirVector - Vector3::UnitY()).norm() < 1e-4) { + } else if ((localDirVector - Vector3::UnitY()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisY; - } else if ((localDirVector - Vector3::UnitZ()).norm() < 1e-4) { + } else if ((localDirVector - Vector3::UnitZ()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisZ;
517-523: More efficient, the rotation check can be!Use quaternion-based comparison for rotation checks, you should. Less computational cost and more numerically stable, this approach is.
Apply this diff to improve rotation comparison:
- if ((vt.localTransform.rotation().matrix() - RotationMatrix3::Identity()) - .norm() > tolerance) { + Quaternion q(vt.localTransform.rotation()); + if (std::abs(q.w()) < 1.0 - tolerance) { ACTS_ERROR("Volumes are not aligned: rotation is different"); throw std::invalid_argument( "Volumes are not aligned: rotation is different"); }
579-882: Too long, this method is. Break it down, we must!Extract smaller, focused methods for better readability and maintainability. Easier to understand and test, the code will be.
Consider extracting these methods:
validateNewBounds()updateOrthogonalBounds()updateDirectionalBounds()handleGapVolumes()Example:
void CuboidVolumeStack::validateNewBounds(const VolumeTuple& oldVolume, const VolumeTuple& newVolume) { constexpr auto tolerance = s_onSurfaceTolerance; auto same = [](double a, double b) { return std::abs(a - b) < tolerance; }; for (const auto& dir : {m_dir, m_dirOrth1, m_dirOrth2}) { validateBoundsInDirection(dir, oldVolume, newVolume, same); } }
499-543: Improve rotation check precision, you should.More precise rotation comparison method, consider using. Matrix comparison with component-wise tolerance, a better approach might be.
Apply this diff to improve rotation check precision:
- if ((vt.localTransform.rotation().matrix() - RotationMatrix3::Identity()) - .norm() > tolerance) { + const auto& R = vt.localTransform.rotation().matrix(); + for (int i = 0; i < 3; ++i) { + for (int j = 0; j < 3; ++j) { + if (std::abs(R(i,j) - (i == j ? 1.0 : 0.0)) > tolerance) { + ACTS_ERROR("Volumes are not aligned: rotation is different"); + throw std::invalid_argument( + "Volumes are not aligned: rotation is different"); + } + } + }
89-103: Remove unnecessary break statements after return, you should.The break statements after return in the switch cases serve no purpose, as the return statement already exits the function. Remove them to improve code clarity.
Apply this diff:
switch (direction) { case AxisDirection::AxisX: return 0; - break; case AxisDirection::AxisY: return 1; - break; case AxisDirection::AxisZ: return 2; - break; default: throw std::invalid_argument("Invalid axis direction"); }
337-338: Remove TODO comment about tolerance, we should.The TODO comment questioning the appropriate tolerance value is unnecessary since the code correctly uses
s_onSurfaceTolerance, which is the established tolerance constant in the codebase.Apply this diff:
- // TODO: What's a good tolerance? constexpr auto tolerance = s_onSurfaceTolerance;
879-880: Clarify TODO comment about transform reuse, we should.The TODO comment about reusing m_transform lacks context and explanation. Either provide more details about the intended optimization or remove the comment if it's not actionable.
Apply this diff:
m_transform = newVolume.globalTransform; - // @TODO: We probably can reuse m_transform m_groupTransform = m_transform;Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (4)
67-339: Long and complex, this test function is. Break it down, we should!Helper functions to extract common validation logic, beneficial would be. Improve readability and maintenance, it would.
Consider extracting these validation blocks into helper functions:
+void validateVolumeBounds(const Volume* volume, + const CuboidVolumeBounds::BoundValues boundDir, + const CuboidVolumeBounds::BoundValues boundDirOrth1, + const CuboidVolumeBounds::BoundValues boundDirOrth2, + double expectedDir, + double expectedOrth1, + double expectedOrth2) { + const auto* cuboidBounds = + dynamic_cast<const CuboidVolumeBounds*>(&volume->volumeBounds()); + BOOST_REQUIRE(cuboidBounds != nullptr); + BOOST_CHECK_EQUAL(cuboidBounds->get(boundDir), expectedDir); + BOOST_CHECK_EQUAL(cuboidBounds->get(boundDirOrth1), expectedOrth1); + BOOST_CHECK_EQUAL(cuboidBounds->get(boundDirOrth2), expectedOrth2); +}
887-914: ASCII art documentation, valuable it is. But its placement, improve we can!Move this documentation to a separate comment block before the test case, we should. Cleaner separation of documentation and code, this would provide.
+/** + * Test scenarios for ResizeGapMultiple: + * + * original size + * <---------------> + * +---------------+ + * | | + * | Volume 1 | + * | | + * +---------------+ + * first resize + * <--------------------------> + * +---------------+----------+ + * | | | + * | Volume 1 | Gap | + * | | | + * +---------------+----------+ + * second resize + * <-----------------------------------> + * +---------------+-------------------+ + * | | | + * | Volume 1 | Gap | + * | | | + * +---------------+-------------------+ + */
1038-1093: Error handling tests, comprehensive they are. But validate error messages, we should!Check the specific error messages thrown, important it is. Better validation of error cases, this provides.
- BOOST_CHECK_THROW(CuboidVolumeStack(volumes, direction, strategy), - std::invalid_argument); + BOOST_CHECK_EXCEPTION( + CuboidVolumeStack(volumes, direction, strategy), + std::invalid_argument, + [](const std::invalid_argument& e) { + return std::string(e.what()).find("Empty volume list") != std::string::npos; + });
65-65: Thread safety tests, missing they are!For concurrent access to CuboidVolumeStack, tests we need. Important for multi-threaded environments, this is.
Add a test case to verify thread safety:
BOOST_AUTO_TEST_CASE(ThreadSafety) { auto vol = std::make_shared<Volume>( Transform3::Identity(), std::make_shared<CuboidVolumeBounds>(100_mm, 400_mm, 400_mm)); std::vector<Volume*> volumes{vol.get()}; CuboidVolumeStack stack(volumes, AxisDirection::AxisZ, VolumeAttachmentStrategy::Gap); std::vector<std::thread> threads; for(int i = 0; i < 10; ++i) { threads.emplace_back([&stack]() { auto bounds = std::make_shared<CuboidVolumeBounds>( dynamic_cast<const CuboidVolumeBounds&>(stack.volumeBounds())); stack.update(bounds, std::nullopt, *logger); }); } for(auto& thread : threads) { thread.join(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Core/include/Acts/Geometry/CuboidVolumeStack.hpp(1 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(4 hunks)Core/include/Acts/Geometry/CylinderVolumeStack.hpp(6 hunks)Core/include/Acts/Geometry/detail/VolumeAttachmentStrategy.hpp(1 hunks)Core/include/Acts/Geometry/detail/VolumeResizeStrategy.hpp(1 hunks)Core/src/Geometry/CMakeLists.txt(2 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(3 hunks)Core/src/Geometry/CuboidVolumeStack.cpp(1 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(4 hunks)Core/src/Geometry/CylinderVolumeStack.cpp(14 hunks)Core/src/Geometry/detail/VolumeAttachmentStrategy.cpp(1 hunks)Core/src/Geometry/detail/VolumeResizeStrategy.cpp(1 hunks)Examples/Python/src/Blueprint.cpp(2 hunks)Examples/Python/src/Geometry.cpp(2 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(4 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeBoundsTests.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp(42 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- Core/src/Geometry/CylinderVolumeStack.cpp
- Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp:28-29
Timestamp: 2024-12-05T20:21:24.253Z
Learning: In `CylinderContainerBlueprintNode`, validation that child volumes are cylinders is enforced by `CylinderVolumeStack`.
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: macos
- GitHub Check: build_debug
🔇 Additional comments (31)
Core/include/Acts/Geometry/CuboidVolumeStack.hpp (1)
100-107: Refactor common utilities, we must.Methods like
axisToIndexandgetOrthogonalAxesduplicate functionality present inCylinderVolumeStack. Extract into a common utility or base class, you should, to promote code reuse and maintainability.Core/include/Acts/Geometry/detail/VolumeResizeStrategy.hpp (1)
15-21: Well-defined, the enumVolumeResizeStrategyis.The enumeration of resize strategies clear and effective it is.
Core/src/Geometry/detail/VolumeAttachmentStrategy.cpp (1)
12-28: Clean and complete, this implementation is!Handle all enum values properly, the switch statement does. Missing default case, a strength it is, hmm, as compiler warnings it will raise if new enum values are added but not handled.
Core/include/Acts/Geometry/detail/VolumeAttachmentStrategy.hpp (1)
17-28: Well documented and type-safe, this enum class is!Clear purpose each strategy has. Type safety the enum class provides. Documentation, thorough it is!
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (2)
75-76: Flexible initialization, this constructor provides!Through initializer list, construct bounds more elegantly, one can. Good addition, this is!
156-159: Wise conversion method, this is!Convert axis direction to bound value in local coordinate convention, this method does. Clear purpose it serves!
Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp (2)
42-45: More generic and reusable, these strategy types have become!From cylinder-specific to generic volume strategies, moved we have. Backward compatibility through default values, preserved it is!
143-147: Consistent with constructor defaults, these member initializations are!Match the constructor parameters, these default values do. Maintain consistency throughout the class, they do!
Core/src/Geometry/CylinderContainerBlueprintNode.cpp (2)
27-28: Approve these changes, I do.Clean and consistent with the new strategy types, this change is.
247-255: Approve these getter methods, I do.Clean and consistent with the strategy type changes, these methods are.
Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp (1)
26-27: Hmm, new headers added, I see.Wise decision it is, to separate volume strategies into dedicated header files. Improved modularity and reusability this brings.
Examples/Python/src/Blueprint.cpp (1)
18-19: New headers, included they are.Consistent with the refactoring of volume strategies, these changes are. Good separation of concerns, I sense.
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp (2)
25-25: Header file added, I notice.Consistent with the strategy refactoring pattern, this change is.
218-218: Updated strategy usage in test, I see.From
CylinderVolumeStack::AttachmentStrategytoVolumeAttachmentStrategy, the transition is made. Test coverage maintained, it is.Examples/Python/src/Geometry.cpp (1)
37-38: New headers, added they are.For volume strategies, dedicated headers now exist. Clean separation of concerns, this brings.
Core/src/Geometry/CylinderVolumeStack.cpp (6)
89-90: Approve these changes, I do!A wise change this is, using more specific strategy types for better type safety and clarity.
108-110: Consistent with the constructor changes, this is!Well aligned with the strategy type changes, maintaining consistency throughout the codebase.
329-330: Harmony in the code, I sense!The Force flows smoothly through these signature changes, maintaining consistency with the strategy pattern.
89-90: Approve the strategy type changes, I do.Aligned with modern strategy management practices, this change is. A cleaner and more maintainable codebase, it promotes.
303-320: Improved logging format, you have.More readable and consistent, the log messages now are. Easier to debug and maintain, this makes it.
Line range hint
356-428: Consistent use of the new strategy type, this is.Well-structured and maintainable, the implementation remains. A good balance between readability and functionality, it strikes.
Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp (2)
49-58: In balance, these test changes are!Updated strategy types in test vectors, aligned with the implementation they are. Strong with the Force, this consistency is!
49-58: Updated test vectors, you have.Consistent with the new strategy types, these changes are. Strong test coverage, they maintain.
Core/src/Geometry/CMakeLists.txt (2)
38-38: Properly integrated, these new components are!Added to the build system, the new source files are. A complete solution, this makes.
Also applies to: 54-55
38-38: Added new source files to the build system, you have.Properly integrated, the new functionality is. Build system organization, this maintains.
Also applies to: 54-55
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (3)
9-34: Well-organized, the includes are! Hmmmm.A clear structure in the include statements, I sense. First the test framework, then the Acts core, and finally the standard library. Good practice, this is!
41-49: Wise design in the test fixture, I observe!RAII principles, this fixture follows. The logging level, it saves and restores. Clean and exception-safe, this approach is!
341-408: Thorough validation of transformations, I see!Both global vector direction and local axis direction, this test compares. Consistency in transformations, it ensures.
Core/src/Geometry/CuboidVolumeStack.cpp (3)
89-103: Safe axis direction handling, this is.Properly handled, all cases are. Clear error messages, it provides.
338-338: Clarify the tolerance choice, you must.Justify why s_onSurfaceTolerance the right choice is, you should. Document the rationale in a comment, you must.
884-889: Clean and focused gap volume creation, this is.Well-managed memory with shared pointers, you have. Simple yet effective implementation, this is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (2)
75-76: Documentation for the constructor, missing it is.Hmmmm. Constructor parameters, documented they must be. Example usage and valid key-value pairs, shown they should be. Help future Padawans understand the ways of the Force, good documentation does.
Add documentation like this, you should:
+ /// Constructor - from key-value pairs + /// + /// @param keyValues Initializer list of pairs mapping BoundValues to their dimensions + /// @throws std::invalid_argument if required bounds are missing + /// + /// Example: + /// @code + /// CuboidVolumeBounds bounds({ + /// {CuboidVolumeBounds::eHalfLengthX, 10.0}, + /// {CuboidVolumeBounds::eHalfLengthY, 20.0}, + /// {CuboidVolumeBounds::eHalfLengthZ, 30.0} + /// }); + /// @endcode
Line range hint
75-159: Strong with the Force, these changes are.Well-designed additions for CuboidVolumeStack support, I see. Flexible initialization and axis handling, good architectural choices they are. Like the ways of the Force, clean and powerful the interface is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Core/include/Acts/Geometry/CuboidVolumeStack.hpp(1 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(4 hunks)Core/src/Geometry/CuboidVolumeStack.cpp(1 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(4 hunks)Core/src/Geometry/detail/VolumeAttachmentStrategy.cpp(1 hunks)Core/src/Geometry/detail/VolumeResizeStrategy.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Core/include/Acts/Geometry/CuboidVolumeStack.hpp
- Core/src/Geometry/CylinderContainerBlueprintNode.cpp
- Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: missing_includes
🔇 Additional comments (8)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (2)
19-19: Explicit include of fundamental types, approve I do.Wise decision it is, to explicitly include . Clear dependencies make, better code they do.
156-159: Improve documentation and verify error handling, we must.For the static method fromAxisDirection, two suggestions I have:
- Document the mapping between AxisDirection and BoundValues, you should. Clear understanding of the Force, it brings.
- Handle invalid directions gracefully, we must. Verify this, we should.
+ /// Convert axis direction to a corresponding bound value + /// in local coordinate convention + /// @param direction the axis direction to convert + /// @returns BoundValues corresponding to the given axis direction + /// @throws std::invalid_argument if direction is not a canonical axis + /// + /// Mapping: + /// - AxisX -> eHalfLengthX + /// - AxisY -> eHalfLengthY + /// - AxisZ -> eHalfLengthZVerify error handling with this script, I shall:
✅ Verification successful
Pleased with the error handling, I am. Documentation improvements alone, we need.
The Force is strong with the error handling, as invalid directions are already caught with std::invalid_argument. Only the documentation improvements suggested earlier, beneficial they would be:
+ /// Convert axis direction to a corresponding bound value + /// in local coordinate convention + /// @param direction the axis direction to convert + /// @returns BoundValues corresponding to the given axis direction + /// @throws std::invalid_argument if direction is not a canonical axis + /// + /// Mapping: + /// - AxisX -> eHalfLengthX + /// - AxisY -> eHalfLengthY + /// - AxisZ -> eHalfLengthZ🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in fromAxisDirection implementation rg "fromAxisDirection.*\{" -A 10Length of output: 34
Script:
#!/bin/bash # Find all files related to CuboidVolumeBounds fd "CuboidVolumeBounds" # Search for the implementation in any related files rg "BoundValues.*CuboidVolumeBounds.*fromAxisDirection" -A 10Length of output: 1164
Script:
#!/bin/bash # Get complete implementation with more context lines rg "fromAxisDirection" Core/src/Geometry/CuboidVolumeBounds.cpp -A 15Length of output: 507
Core/src/Geometry/detail/VolumeAttachmentStrategy.cpp (1)
12-31: Clean and complete, this implementation is!Handle all attachment strategies wisely, it does. Default case for unknown values, included it has. Pleased with this implementation, I am.
Core/src/Geometry/detail/VolumeResizeStrategy.cpp (1)
13-26: Well structured, this implementation is!Follow good patterns, it does. Handle all cases with grace, including the unknown, it does. Consistent with its sibling strategy, it remains.
Core/src/Geometry/CuboidVolumeStack.cpp (4)
146-151: 🛠️ Refactor suggestionConsistent tolerance values, use we must!
The hardcoded tolerance of 1e-4, replaced with s_onSurfaceTolerance it should be. Maintain consistency throughout the codebase, this will.
Apply this diff:
- if ((localDirVector - Vector3::UnitX()).norm() < 1e-4) { + if ((localDirVector - Vector3::UnitX()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisX; - } else if ((localDirVector - Vector3::UnitY()).norm() < 1e-4) { + } else if ((localDirVector - Vector3::UnitY()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisY; - } else if ((localDirVector - Vector3::UnitZ()).norm() < 1e-4) { + } else if ((localDirVector - Vector3::UnitZ()).norm() < s_onSurfaceTolerance) { m_dir = AxisDirection::AxisZ;Likely invalid or redundant comment.
47-58: 🛠️ Refactor suggestionMore careful with floating-point calculations, we must be!
In the calculations of min and max, numerical precision issues may arise. Use tolerance-based comparisons or nextafter, we should.
Apply this diff to improve floating-point handling:
double min(AxisDirection direction) const { - return mid(direction) - halfLength(direction); + return std::nextafter(mid(direction) - halfLength(direction), -std::numeric_limits<double>::infinity()); } double max(AxisDirection direction) const { - return mid(direction) + halfLength(direction); + return std::nextafter(mid(direction) + halfLength(direction), std::numeric_limits<double>::infinity()); }Likely invalid or redundant comment.
89-103: 🛠️ Refactor suggestionSafer handling of axis directions, implement we must!
Rely on internal enum values directly, we should not. A more explicit mapping through a switch statement, safer it would be.
Consider using a lookup table or explicit mapping:
std::size_t axisToIndex(AxisDirection direction) { + static const std::unordered_map<AxisDirection, std::size_t> axisMap = { + {AxisDirection::AxisX, 0}, + {AxisDirection::AxisY, 1}, + {AxisDirection::AxisZ, 2} + }; + auto it = axisMap.find(direction); + if (it == axisMap.end()) { + throw std::invalid_argument("Invalid axis direction"); + } + return it->second; - switch (direction) { - case AxisDirection::AxisX: - return 0; - break; - case AxisDirection::AxisY: - return 1; - break; - case AxisDirection::AxisZ: - return 2; - break; - default: - throw std::invalid_argument("Invalid axis direction"); - } }Likely invalid or redundant comment.
429-461: 🛠️ Refactor suggestionValidate the gap dimensions, we must!
Missing checks for minimum gap size and valid bounds, I see. Add validation to prevent creation of degenerate gaps, you should.
Apply this diff to add validation:
case VolumeAttachmentStrategy::Gap: { ACTS_VERBOSE(" -> Strategy: Create a gap volume"); double gapHl = (b.min(m_dir) - a.max(m_dir)) / 2.0; + if (gapHl < s_onSurfaceTolerance) { + ACTS_ERROR("Gap size too small: " << gapHl); + throw std::invalid_argument("Gap size below tolerance"); + } double gapMid = (b.min(m_dir) + a.max(m_dir)) / 2.0;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
Core/src/Geometry/CylinderVolumeStack.cpp (1)
Line range hint
1-1089: Architectural wisdom, share I must.Consider these improvements for the future:
- Extract gap volume creation logic into separate strategy classes, we should
- Reduce cognitive complexity in the update method, we must
- Add property-based testing for volume transformations, beneficial it would be
Yet solid and well-documented, the current implementation is. Proceed with confidence, we can.
Core/src/Geometry/CuboidVolumeBounds.cpp (2)
12-12: Remove unused header, I must.Unnecessary inclusion of LineSurface.hpp, I sense. In this implementation, used it is not.
-#include "Acts/Surfaces/LineSurface.hpp"
37-50: Improve constructor implementation, we shall.Robust your implementation is, but room for improvement there still remains:
- More descriptive error message provide, you should
- More efficient initialization with std::fill, use you could
CuboidVolumeBounds::CuboidVolumeBounds( std::initializer_list<std::pair<BoundValues, double>> keyValues) { - m_values = {-1, -1, -1}; + std::fill(m_values.begin(), m_values.end(), -1); for (const auto& [key, value] : keyValues) { m_values[key] = value; } // Throw error here instead of consistency check for clarity if (std::any_of(m_values.begin(), m_values.end(), [](const auto& val) { return val == -1; })) { - throw std::logic_error("Missing bound values"); + throw std::logic_error("Missing cuboid bound values: all dimensions (X, Y, Z) must be provided"); } checkConsistency(); buildSurfaceBounds(); }Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (5)
39-39: Improve logger initialization with configuration options, hmm!A more flexible approach to logger initialization, consider we must. Allow configuration of logging level through test parameters or environment variables, we should.
-auto logger = Acts::getDefaultLogger("UnitTests", Acts::Logging::VERBOSE); +auto logger = Acts::getDefaultLogger("UnitTests", + Acts::Logging::Level(std::getenv("ACTS_LOG_LEVEL") ? + std::stoi(std::getenv("ACTS_LOG_LEVEL")) : + Acts::Logging::VERBOSE));
41-49: Const correctness improve, we must!The member variable
m_level, mark as const after initialization, we should. Prevent accidental modifications during test execution, this will.- Logging::Level m_level; + const Logging::Level m_level; Fixture() : m_level(Acts::Logging::getFailureThreshold()) { Acts::Logging::setFailureThreshold(Acts::Logging::FATAL); }
67-339: BaselineLocal test case, document better we must!Comprehensive test case this is, but documentation of test parameters and their significance, lacking it is. Add comments explaining the purpose of each parameter and expected outcomes, we should.
+// Test the CuboidVolumeStack with various configurations: +// - angle: Rotation angle around orthogonal axis (-135° to 180° in 45° steps) +// - rotate: Volume order rotation (0 or 1) +// - shift: Volume spacing factor (0.8, 1.0, 1.2) +// - offset: Stack position offset +// - strategy: Volume attachment strategy +// - dir: Stacking direction (X, Y, or Z axis) BOOST_DATA_TEST_CASE(BaselineLocal, (boost::unit_test::data::xrange(-135, 180, 45) *
1017-1036: Test error cases more thoroughly, we must!In InvalidDirection test, additional assertions for error message content, add we should. Verify specific error conditions, this will.
BOOST_CHECK_THROW(CuboidVolumeStack(volumes, AxisDirection::AxisR, strategy, - std::invalid_argument); + std::invalid_argument, + "Expected error message: Invalid axis direction AxisR");
1-1122: Unit test coverage, incomplete it appears!Missing test cases for edge conditions and error scenarios, I sense:
- Test with empty bounds or zero dimensions
- Test with negative dimensions
- Test with extremely large dimensions
- Test with NaN or infinite values in transformations
Help you implement these test cases, shall I?
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/src/Geometry/CuboidVolumeBounds.cpp(3 hunks)Core/src/Geometry/CylinderVolumeStack.cpp(13 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (5)
Core/src/Geometry/CylinderVolumeStack.cpp (4)
89-90: Hmm, renamed strategy parameters I see.Clean and consistent with the new naming convention, this change is. Approve it, I do.
108-110: Aligned with the constructor changes, this signature update is.Maintain consistency throughout the codebase, we must. Good, this change is.
355-355: Updated switch cases for attachment strategies, I observe.Handle all cases correctly, the implementation does:
- Midpoint strategy at line 355
- First volume strategy at line 390
- Second volume strategy at line 409
- Gap creation strategy at line 427
Thorough error handling for unknown strategies, there is. Pleased with this implementation, I am.
Also applies to: 390-390, 409-409, 427-427
790-790: Resize strategy implementation, robust it is.Handle volume expansion and gap creation wisely, the code does:
- Expand strategy for Z-direction at line 790
- Gap strategy for Z-direction at line 824
- Expand strategy for R-direction at line 949
- Gap strategy for R-direction at line 972
Maintain volume integrity during resizing operations, it does. Prevent shrinking of volumes, it does. Strong with the Force, this implementation is.
Also applies to: 824-824, 949-949, 972-972
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (1)
915-1015: ResizeGapMultiple test case, well documented it is!ASCII art diagram explaining the test case, appreciate I do! Clear understanding of the test scenario, it provides. Continue this practice for other complex test cases, you should.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (5)
9-16: Optimize your includes, young padawan must.Redundant Boost test includes, I sense. Only
<boost/test/unit_test.hpp>, need you may, as it includes most basic test functionality.-#include <boost/test/data/test_case.hpp> -#include <boost/test/detail/log_level.hpp> -#include <boost/test/tools/context.hpp> -#include <boost/test/tools/old/interface.hpp> -#include <boost/test/unit_test.hpp> -#include <boost/test/unit_test_log.hpp> -#include <boost/test/unit_test_parameters.hpp> -#include <boost/test/unit_test_suite.hpp> +#include <boost/test/unit_test.hpp> +#include <boost/test/data/test_case.hpp>
41-49: Document the ways of the Force, we must.Missing documentation for the Fixture class, I see. Its purpose and behavior, explain you should.
+/** + * Test fixture that temporarily modifies the logging threshold. + * Sets threshold to FATAL during test execution and restores + * the original threshold afterwards. + */ struct Fixture { Logging::Level m_level; Fixture() { m_level = Acts::Logging::getFailureThreshold();
67-339: Clearer documentation for BaselineLocal test, write you must.Complex test case with many parameters, this is. Document its purpose and parameters, we should.
+/** + * Tests the baseline functionality of CuboidVolumeStack with local coordinates. + * + * Parameters: + * @param angle - Rotation angle in degrees + * @param rotate - Rotation index for volume order + * @param shift - Volume spacing factor + * @param offset - Base translation offset + * @param strategy - Volume attachment strategy + * @param dir - Stacking direction + */ BOOST_DATA_TEST_CASE(BaselineLocal, (boost::unit_test::data::xrange(-135, 180, 45) *
887-914: Helpful ASCII art, you have drawn. Improve it further, we can.Good visual representation, but clearer it could be. Add dimensions and scale to diagram, suggest I do.
-// original size -// <---------------> +// original size (200 units) +// <------------------------> // +---------------+ // | | // | | // | Volume 1 | // | | // | | // +---------------+ -// first resize -// <--------------------------> +// first resize (300 units) +// <-------------------------------->
915-1015: Strong with the Force, this test is. But clearer names, it needs.More descriptive name for test case, suggest I do. 'ResizeGapMultiple' unclear it is.
-BOOST_DATA_TEST_CASE(ResizeGapMultiple, +BOOST_DATA_TEST_CASE(TestGapVolumeReuseOnMultipleResizes,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (1)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (1)
1-1122: Well-crafted test suite, this is.Comprehensive test coverage and robust test cases, you have created. Minor improvements in documentation and organization suggested, but strong with the Force, this code is.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (5)
41-49: Document the purpose of the Fixture class, hmm.Clear documentation for the Fixture class, missing it is. Explain why logging threshold manipulation needed it is, future maintainers will thank you they will.
Add this documentation, you should:
struct Fixture { + // Temporarily adjusts the logging threshold during tests to suppress non-fatal + // log messages, ensuring clean test output it does. Logging::Level m_level;
53-63: Document the strategy vectors, you must.Understanding of these strategies, crucial it is. Document their purpose and impact on the tests, we should.
Add documentation like this, you shall:
+// Strategies for handling volume attachments in the stack static const std::vector<VolumeAttachmentStrategy> strategies = { VolumeAttachmentStrategy::Gap, VolumeAttachmentStrategy::First, VolumeAttachmentStrategy::Second, VolumeAttachmentStrategy::Midpoint, }; +// Strategies for handling volume resizing in the stack static const std::vector<VolumeResizeStrategy> resizeStrategies = { VolumeResizeStrategy::Expand, VolumeResizeStrategy::Gap, };
67-80: Document the test parameters, essential it is.Understanding the purpose of each test parameter, difficult it is without documentation. Help future maintainers, we must.
Add parameter documentation like this, you should:
+// Tests the baseline functionality of CuboidVolumeStack with local coordinates +// Parameters: +// angle: Rotation angle in degrees for volume transformation +// rotate: Index to rotate volumes array, testing order independence +// shift: Scale factor for volume spacing +// offset: Translation vector for base transformation +// strategy: Volume attachment strategy to test +// dir: Axis direction for stacking BOOST_DATA_TEST_CASE(BaselineLocal, (boost::unit_test::data::xrange(-135, 180, 45) * boost::unit_test::data::xrange(0, 2, 1) *
484-496: Extract magic numbers into named constants, we should.Harder to maintain and understand, magic numbers are. Named constants, more readable and maintainable they make the code.
Move these to named constants at the top of the test suite, you should:
+// Test constants for UpdateStack +constexpr double kHalfLength = 400_mm; +constexpr double kOrthLength1 = 100_mm; +constexpr double kOrthLength2 = 600_mm; +constexpr double kAngleStart = -135; +constexpr double kAngleEnd = 180; +constexpr double kAngleStep = 45; BOOST_DATA_TEST_CASE(UpdateStack, - (boost::unit_test::data::xrange(-135, 180, 45) * + (boost::unit_test::data::xrange(kAngleStart, kAngleEnd, kAngleStep) *
893-920: Well documented with ASCII art, this test case is! Improve formatting slightly, we can.Helpful for understanding the test case, this ASCII art is. Small improvements in formatting and organization, suggest I do.
Format the ASCII art more consistently, you should:
-// original size -// <---------------> +// Initial configuration: +// <----------------> // +---------------+ // | | // | | // | Volume 1 | // | | // | | // +---------------+ -// first resize +// After first resize: // <-------------------------->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (1)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (1)
1125-1128: Properly closed, the test suites and namespace are.Clean and well-organized, the code structure is. Approve this, I do.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Core/src/Geometry/CuboidVolumeBounds.cpp (3)
12-12: Unnecessary include, I sense.The Force tells me that
Acts/Surfaces/LineSurface.hppis included, yet used it is not. Remove it, we should, unless future changes require it.
37-50: Optimize the constructor's path, we must.Good the implementation is, but improve it further, we can:
- Use
std::fillfor initialization, more efficient it is:- : m_values({-1, -1, -1}) { + : m_values() { + std::fill(m_values.begin(), m_values.end(), -1);
- Combine the checks, reduce complexity we should:
- if (std::any_of(m_values.begin(), m_values.end(), - [](const auto& val) { return val == -1; })) { - throw std::logic_error("Missing bound values"); - } - checkConsistency(); + for (const auto& val : m_values) { + if (val <= 0) { + throw std::logic_error(val == -1 ? "Missing bound values" + : "Invalid input, zero or negative"); + } + }
37-50: Document the ways of the Force, you must.Strong the implementation is, but documentation it lacks. Add Doxygen comments for new methods, explain parameters and exceptions, help future Padawans understand the code's purpose, we should.
Also applies to: 172-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Geometry/CuboidVolumeBounds.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: smearing_config
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: macos
- GitHub Check: lint
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: linux_ubuntu
- GitHub Check: docs
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: build_debug
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
🔇 Additional comments (1)
Core/src/Geometry/CuboidVolumeBounds.cpp (1)
172-185: Previous wisdom, still applies it does.Hmmmm... A previous review comment, I see. The same concerns, they remain. Make this
constexpr, remove redundant breaks, and use proper enum class syntax, we must.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Examples/Python/src/Geometry.cpp (1)
296-304: Hmm, good refactoring, I sense in these enum bindings!Moved from CylinderVolumeStack to top-level scope, these enums have been. A path to greater flexibility in volume management, this creates. Support the new CuboidVolumeStack feature without duplicating code, it will.
Consider documenting these enums in Python docstrings, help future Padawans understand their purpose, we must. Add them, shall we?
Core/include/Acts/Geometry/VolumeResizeStrategy.hpp (1)
15-21: Strong with the Force, this enum definition is! Yet more wisdom, we could share.Clear documentation you have provided, young Padawan. But enhance it further we could, with examples of when each strategy to use.
Add usage examples to the documentation, like this you should:
/// The resize strategy defines how the volumes are resized +/// Usage example: +/// - Use Expand when continuous detector volumes you need +/// - Use Gap when separate volumes for support structure you must create enum class VolumeResizeStrategy {Core/include/Acts/Geometry/VolumeAttachmentStrategy.hpp (2)
15-28: Clear the documentation is, yet strengthen it further we must!Speak of local coordinates you do, but clarify the coordinate system conventions fully, we should. Help young Padawans understand the way of the Force better, this will.
Add coordinate system clarification, like this you should:
/// The attachment strategy defines how the volumes are attached /// Attachment always happens pair-wise +/// Note: All coordinates referenced below are in the local coordinate system +/// where: +/// - x: points along the detector's length +/// - y: points upward +/// - z: completes the right-handed coordinate system enum class VolumeAttachmentStrategy {
18-23: Improve the clarity of First and Second strategies, we must!Confusion about "left" and "right" terms, prevent we should. More precise in our language, we must be.
Clarify the directional terms, like this:
- /// Given two volumes, the *left* one, i.e. the one with the lower **local** - /// x, y, or z value is extended + /// Given two volumes, the first one (with lower coordinate value in the + /// specified direction) is extended to bridge the gap First, - /// Given two volumes, the *right* one, i.e. the one with the higher - /// **local** x, y, or z value is extended + /// Given two volumes, the second one (with higher coordinate value in the + /// specified direction) is extended to bridge the gap Second,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Core/include/Acts/Geometry/CuboidVolumeStack.hpp(1 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(4 hunks)Core/include/Acts/Geometry/CylinderVolumeStack.hpp(6 hunks)Core/include/Acts/Geometry/VolumeAttachmentStrategy.hpp(1 hunks)Core/include/Acts/Geometry/VolumeResizeStrategy.hpp(1 hunks)Core/src/Geometry/CMakeLists.txt(2 hunks)Core/src/Geometry/VolumeAttachmentStrategy.cpp(1 hunks)Core/src/Geometry/VolumeResizeStrategy.cpp(1 hunks)Examples/Python/src/Blueprint.cpp(2 hunks)Examples/Python/src/Geometry.cpp(2 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(4 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp(42 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/UnitTests/Core/Geometry/BlueprintTests.cpp
- Examples/Python/src/Blueprint.cpp
- Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp
- Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (15)
Examples/Python/src/Geometry.cpp (1)
37-38: Wise addition of headers, this is!Correctly placed and ordered, the new volume strategy headers are. Support the generalization of volume stacking functionality, they do.
Core/src/Geometry/VolumeResizeStrategy.cpp (1)
13-26: Hmmmm, well implemented this stream operator is!Clean and robust, the implementation appears. All cases handled properly they are, and protection against future enum values, the default case provides. Return the stream correctly, it does.
Core/src/Geometry/VolumeAttachmentStrategy.cpp (1)
12-31: Balanced and harmonious, like the Force itself, this implementation is!Mirror the style of VolumeResizeStrategy, it does. Wise decision, this consistency is. Protected against future changes through default case, we remain.
Core/src/Geometry/CMakeLists.txt (1)
38-38: Approve these changes, I do.Properly added to the build system, the new source files are. Maintained, the alphabetical ordering is.
Also applies to: 54-55
Core/include/Acts/Geometry/CylinderVolumeStack.hpp (3)
14-15: Added correctly, the new strategy headers are.Properly included, the new volume strategy headers are, replacing the old enum declarations.
56-57: Updated constructor parameters, I see.Replaced with new strategy types, the parameters are. Maintained, the default values are.
90-92: Consistent with the new types, these changes are.Throughout the class, updated to use new strategy types, the method signatures and member variables are. A cohesive refactoring, this represents.
Also applies to: 166-166
Core/include/Acts/Geometry/CuboidVolumeStack.hpp (3)
52-56: Mindful of ownership semantics, be.Passing
std::vector<Volume*>& volumesby non-const reference and storing it as member variable, dangerous it can be. Consider managing ownership more explicitly or documenting lifetime requirements.
90-92: Unnecessary copying of shared pointers, avoid we should.Pass
volboundsandtransformby const reference, we must, to prevent unnecessary copying of shared pointers.
98-107: Well designed, these utility methods are.Wisely added, the static helper methods
axisToIndexandgetOrthogonalAxesare. Clear purpose and good abstraction, they provide.Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (3)
67-340: Comprehensive, this baseline test is.Well structured and thorough, the BaselineLocal test is. All key aspects it covers:
- Volume transformations
- Bounds synchronization
- Gap creation
- Strategy behaviors
921-1021: Clear and well-documented, this test case is.Excellent documentation with ASCII art, the ResizeGapMultiple test has. Understanding the test scenarios, easier it makes.
1023-1042: Proper error handling verification, this is.Correctly tests invalid direction handling, this does. Important for robust implementation, such validation is.
Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp (2)
49-58: Updated correctly, the strategy definitions are.Properly migrated to new strategy types, the test vectors are. Maintained, the test coverage is.
1791-1792: Consistently updated, these test cases are.Throughout the test file, properly replaced with new strategy types, all constructor calls are. No functional changes required, this migration was.
Also applies to: 1811-1812, 1846-1846, 1867-1867, 1888-1888
|
acts-project-service
added
the
Breaks Athena build
Adding
CuboidVolumeStack. The overall logic is similar toCylinderVolumeStack-- a set of aligned volumes is formed into a stack, the stack volumes are adjusted in size and the connection between the volumes is insured via resizing/construction of the gap volumes.The main difference with
CylinderVolumeStackis in the fact that cuboid volumes don't really have a "special" direction that can fix the stacking conventions, e.g. specific treatment ofAxisZandAxisRstacking. For this reason a level of generalization is applied to the stacking direction handling, including handling of arbitrary (in global coordinate system) merging directions.Summary by CodeRabbit
Release Notes
New Features
CuboidVolumeStackclass for managing stacks of cuboid volumes.VolumeAttachmentStrategyandVolumeResizeStrategyfor flexible volume management.CuboidVolumeBoundswith a new constructor for flexible initialization.Improvements
Testing
Documentation