feat: introducing common ProtoAxis concept #4034
Conversation
WalkthroughEnhance the axis management utilities in the ACTS project, this pull request does. A virtual destructor added to the Changes
Possibly related PRs
Suggested Reviewers
Poem
✨ 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: 0
🧹 Nitpick comments (7)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (4)
90-92: A typo in the error message, I sense!"somethgin" instead of "something", written it is. Fix this, we must!
- // Invalid constructor, closed with somethgin else than phi or rphi + // Invalid constructor, closed with something else than phi or rphi
124-125: Again, the typo appears!Like a persistent disturbance in the Force, "somethgin" returns!
- // Invalid constructor, closed with somethgin else than phi or rphi + // Invalid constructor, closed with something else than phi or rphi
128-128: A disturbance in the test name, I sense!"VariabletProtoAxis" written it is, but "VariableProtoAxis" correct it should be!
-BOOST_AUTO_TEST_CASE(VariabletProtoAxis) { +BOOST_AUTO_TEST_CASE(VariableProtoAxis) {
17-180: Refactoring opportunity, I sense!Common test patterns across test cases, I see. Create helper functions for repeated validations, we should:
- Axis property verification
- Invalid constructor testing
- String representation checking
Reduce code duplication and improve maintainability, this will!
Help you create these helper functions, shall I? Share your wisdom on this matter, you must!
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
78-85: Copy and move semantics, consider them you should.Unique pointers you use; copying defaulted it is not. Explicitly delete copy constructor and assignment operator, perhaps you should. If moving needed it is, default move operations you may.
Apply this diff to prevent unintended copying:
+ ProtoAxis(const ProtoAxis&) = delete; + ProtoAxis& operator=(const ProtoAxis&) = delete; + + ProtoAxis(ProtoAxis&&) = default; + ProtoAxis& operator=(ProtoAxis&&) = default;Core/src/Utilities/ProtoAxis.cpp (1)
36-42: Initialization of m_axis in autorange constructor, reconsider we should.Placeholder range [0, 1] you use. Confusing it might be, if actual range differs. Delay creation of m_axis until range known, perhaps.
Alternative constructor without initializing m_axis, consider:
- m_axis(IAxis::create(abType, 0., 1., nbins)), - m_autorange(true) { + m_axis(nullptr), + m_autorange(true) {Initialize m_axis when actual range determined, you can.
Core/include/Acts/Utilities/IAxis.hpp (1)
65-75: Factory method for equidistant binning, well designed it is!Clean interface for axis creation, this provides. Smart pointers for memory management, it uses wisely.
Consider documenting the ownership transfer semantics in the method comments, helpful for future Padawans this would be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Core/include/Acts/Utilities/IAxis.hpp(3 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Core/src/Utilities/IAxis.cpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)Tests/UnitTests/Core/Utilities/AxesTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Core/src/Utilities/CMakeLists.txt
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ 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: CI Bridge / clang_tidy
- GitHub Check: macos
- GitHub Check: build_debug
- GitHub Check: missing_includes
🔇 Additional comments (16)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (3)
1-17: Proper structure and includes, hmmmm, yes!Well-organized the file structure is, with necessary headers and proper licensing it has. Strong with the Force, this implementation is!
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
19-93: Comprehensive testing, impressed I am!Cover all aspects of equidistant axes, these tests do:
- Bound and open axes
- Grid creation for both 1D and 2D
- Invalid constructor cases
- Property verification
Strong with the testing Force, you are!
128-178: Most impressive, the variable axis testing is!Thoroughly tested all aspects are:
- Axis properties
- Edge cases
- Invalid directions for closed axes
- Boundary conditions
A true Jedi Master of testing, the author is!
Core/src/Utilities/IAxis.cpp (4)
22-32: Proper input validation, implemented it is.Robust the checks are, and informative exceptions they throw.
34-43: Handle unknown boundary types, you wisely do.Logical error handling, ensure it does. Good, this is.
46-71: Validation of variable bin edges, satisfactory it is.Errors caught they are, if edges insufficient or unsorted.
19-20: Ensure C++20 support, we must.'Using enum' syntax a C++20 feature is. Confirm that the project's compiler settings enable C++20, you should.
Run this script to check the C++ standard in the CMake files:
✅ Verification successful
Strong with C++20, this project is. Safe to use 'using enum', you are!
The Force is with your code, for C++20 support explicitly enforced in
cmake/ActsCompilerOptions.cmake, it is. Fail to build with lesser standards, it will.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if C++20 is enabled in CMake settings. # Search for CMAKE_CXX_STANDARD in CMakeLists.txt rg 'CMAKE_CXX_STANDARD' --glob '**/CMakeLists.txt'Length of output: 50
Script:
#!/bin/bash # Search for various C++ standard related patterns echo "=== Searching for C++ standard settings ===" rg -i "c\+\+\s*(17|20|2a|latest)" --type cmake echo -e "\n=== Searching for compiler flags ===" rg "CMAKE_CXX_FLAGS|add_compile_options|set\(.*FLAGS" --type cmake echo -e "\n=== Looking for build system files ===" fd -e vcxproj -e props echo -e "\n=== Checking root CMakeLists.txt ===" if [ -f "CMakeLists.txt" ]; then cat CMakeLists.txt fiLength of output: 22304
Script:
#!/bin/bash # Check ActsCompilerOptions.cmake for C++ standard settings echo "=== Checking ActsCompilerOptions.cmake ===" cat cmake/ActsCompilerOptions.cmakeLength of output: 2435
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
27-85: Well designed, the ProtoAxis class is.Clear constructors and member functions, you have. Logical and maintainable, the code appears.
95-122: Helper functions for grid creation, effective they are.Template usage and visitor pattern, well applied they are. Flexibility to create grids, this provides.
Core/src/Utilities/ProtoAxis.cpp (2)
12-21: Consistency checks, wisely implemented they are.Invalid combinations of axis direction and boundary type, caught they are. Prevent errors at runtime, this will.
60-73: String representation, informative it is.Useful for debugging and logging, the output will be. Good addition, this is.
Core/include/Acts/Utilities/IAxis.hpp (2)
24-25: Wise addition of virtual destructor, this is!Proper cleanup of derived classes through base pointer, this ensures. Memory leaks, it prevents.
77-84: Factory method for variable binning, good design principles it follows!Consistent with equidistant factory method, this implementation is. Strong exception guarantees, it provides.
Tests/UnitTests/Core/Utilities/AxesTests.cpp (2)
595-624: Comprehensive test coverage for factory methods, achieved you have!Test all boundary types and axis variations, you do. Wise approach to validation, this is.
625-636: Strong error handling tests, implemented you have!Invalid cases thoroughly tested:
- When minimum exceeds maximum
- When bin count is zero
- When edge count insufficient is
- When edges improperly ordered are
Defensive programming practices, these tests enforce.
Tests/UnitTests/Core/Utilities/CMakeLists.txt (1)
30-30: Properly integrated the new test module is!Alphabetical order maintained, it has. Build system integration correct, it is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
17-54: Enhance the documentation clarity, young padawan must.Documentation improvements suggest I do:
- For constructor parameters, validation rules document you should
- For
nbins, minimum value specify you must- For
edges, ordering requirements clarify you shall
109-127: Validate axis compatibility, you must.Before grid creation, compatibility between axes verify you should. Different boundary types or overlapping directions, handle gracefully you must.
template <typename payload_t> std::unique_ptr<IGrid> makeGrid(const ProtoAxis& a, const ProtoAxis& b) { + // Validate axis compatibility + if (a.getAxisDirection() == b.getAxisDirection()) { + throw std::invalid_argument("Axes must have different directions"); + } return a.getAxis().visit([&](const auto& axisA) -> std::unique_ptr<IGrid> {Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (2)
19-93: More edge cases test, you shall.Additional test cases suggest I do:
- Extreme values for min/max test you should
- Numerical precision edge cases verify you must
- Grid creation with non-default payload types validate you shall
95-126: Test more autorange scenarios, you must.Missing test cases observe I do:
- Behavior with negative bin counts verify you should
- Edge cases for boundary types validate you must
- Performance with large bin counts measure you shall
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: docs
🔇 Additional comments (3)
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
56-91: Wise implementation of move semantics, this is!Follow modern C++ best practices, your code does. Properly deleted copy operations and defaulted move operations, you have. Clear purpose, your protected members serve.
93-107: Handle errors gracefully, you must.For invalid payload types or axis configurations, proper error handling implement you should. Consider throwing meaningful exceptions when grid creation fails.
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
128-178: Strong with the Force, these tests are!Comprehensive testing of variable binning, you have achieved. Invalid configurations catch well, you do. Thorough validation of axis directions, implemented you have.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Core/src/Utilities/ProtoAxis.cpp (2)
18-23: More efficient error message construction, you must have.Use std::format for better performance and readability, hmm, yes.
- std::string msg = - "ProtoBinning: Invalid axis boundary type 'Closed' for direction '"; - msg += axisDirectionName(aDir) + - "'. Closed boundary type is only valid for " - "AxisPhi and AxisRPhi directions."; + std::string msg = std::format( + "ProtoBinning: Invalid axis boundary type 'Closed' for direction '{}'. " + "Closed boundary type is only valid for AxisPhi and AxisRPhi directions.", + axisDirectionName(aDir));
74-76: Consistent string formatting, we need.Replace multiple string concatenations with a single std::format call, improve readability and performance it will.
- return std::string("ProtoAxis: ") + std::to_string(getAxis().getNBins()) + - std::string(" bins in ") + axisDirectionName(m_axisDir) + - std::string(", ") + axisType + std::string(" ") + rangeStr; + return std::format("ProtoAxis: {} bins in {}, {} {}", + getAxis().getNBins(), + axisDirectionName(m_axisDir), + axisType, + rangeStr);Core/include/Acts/Utilities/ProtoAxis.hpp (3)
56-59: Documentation for move operations, missing it is.Explain why copy operations are deleted and move operations are defaulted, you should.
Add documentation comments:
+ /// Copy operations are deleted as the class owns a unique_ptr ProtoAxis(const ProtoAxis&) = delete; ProtoAxis& operator=(const ProtoAxis&) = delete; + /// Move operations are defaulted for efficient transfer of axis ownership ProtoAxis(ProtoAxis&&) = default; ProtoAxis& operator=(ProtoAxis&&) = default;
120-123: More informative error message for axis direction conflict, provide we must.Include the actual conflicting directions in the error message, helpful it would be.
if (a.getAxisDirection() == b.getAxisDirection()) { throw std::invalid_argument( - "ProtoAxis::makeGrid: Axes must have different directions"); + std::format("ProtoAxis::makeGrid: Both axes have same direction '{}', different directions required", + axisDirectionName(a.getAxisDirection()))); }
95-95: Fix typo in documentation, we shall."payloat" to "payload", corrected it must be.
-/// @tparam payload_t the grid payloat type +/// @tparam payload_t the grid payload typeTests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
17-17: Test move operations, we must.Missing tests for move constructor and move assignment operator, I sense. Add them, we should.
Add test case:
BOOST_AUTO_TEST_CASE(MoveOperations) { using enum Acts::AxisBoundaryType; using enum Acts::AxisDirection; Acts::ProtoAxis original(AxisX, Bound, 0.0, 1.0, 10); // Test move constructor Acts::ProtoAxis moved(std::move(original)); BOOST_CHECK_EQUAL(moved.getAxisDirection(), AxisX); BOOST_CHECK_EQUAL(moved.getAxis().getNBins(), 10); // Test move assignment Acts::ProtoAxis assigned(AxisY, Bound, 0.0, 2.0, 5); assigned = std::move(moved); BOOST_CHECK_EQUAL(assigned.getAxisDirection(), AxisX); BOOST_CHECK_EQUAL(assigned.getAxis().getNBins(), 10); }🧰 Tools
🪛 cppcheck (2.10-2)
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_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 (5)
Core/include/Acts/Utilities/IAxis.hpp(3 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)Tests/UnitTests/Core/Utilities/AxesTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/UnitTests/Core/Utilities/AxesTests.cpp
- Core/include/Acts/Utilities/IAxis.hpp
🧰 Additional context used
📓 Learnings (2)
Core/src/Utilities/ProtoAxis.cpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: merge-sentinel
- 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
- GitHub Check: unused_files
- 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
🔇 Additional comments (1)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
19-95: Well-written and comprehensive tests, these are.Good coverage of functionality and error cases, you have. Using enum class for improved readability, wise choice it is.
Also applies to: 97-128, 130-180
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/src/Utilities/ProtoAxis.cpp (2)
13-26: Improve error message construction, we must.More efficient and readable, using std::ostringstream would be. String concatenation, a path to the dark side it is.
Apply this change, you should:
- std::string msg = - "ProtoBinning: Invalid axis boundary type 'Closed' for direction '"; - msg += axisDirectionName(aDir) + - "'. Closed boundary type is only valid for " - "AxisPhi and AxisRPhi directions."; + std::ostringstream msg; + msg << "ProtoBinning: Invalid axis boundary type 'Closed' for direction '" + << axisDirectionName(aDir) + << "'. Closed boundary type is only valid for " + "AxisPhi and AxisRPhi directions."; - throw std::invalid_argument(msg); + throw std::invalid_argument(msg.str());
60-78: Virtual, the toStream method should be.For future Jedi to extend this class, virtual toStream would serve better. In the header file, this change make:
// In ProtoAxis.hpp virtual void toStream(std::ostream& os) const;Strong and extensible, this makes the class. Hmmmm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Utilities/ProtoAxis.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
Core/src/Utilities/ProtoAxis.cpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (3)
Core/src/Utilities/ProtoAxis.cpp (3)
1-11: Proper license and includes, I see.Clean and minimal, the includes are. Well-organized, the code structure is.
28-46: Well-crafted constructors, these are.Validation in IAxis::create handled is, as learned from the past we have. Proper initialization and consistency checks, maintained they are. The Force, strong with this code it is.
48-58: Simple and effective, these getters are.Clear purpose they have, const-correctness they maintain. The way of the Jedi, this is.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (2)
19-99: Comprehensive tests you have written, but strengthen them further, we can!Consider these additional test cases, young padawan:
- Test behavior at exact bin edges
- Validate bin center calculations
- Verify behavior with negative bin counts
Help you implement these additional test cases, shall I?
134-184: Explicit about bin ordering, we must be!Test the following scenarios, you should:
- Verify strictly increasing bin edges
- Validate behavior with duplicate bin edges
Add these tests, like so:
+ // Test strictly increasing bin edges + BOOST_CHECK_THROW( + Acts::ProtoAxis(AxisX, Bound, {0.0, 2.0, 1.0}), + std::invalid_argument); + + // Test duplicate bin edges + BOOST_CHECK_THROW( + Acts::ProtoAxis(AxisX, Bound, {0.0, 1.0, 1.0, 2.0}), + std::invalid_argument);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (26)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: build_debug
- GitHub Check: macos
- GitHub Check: docs
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx
🔇 Additional comments (2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (2)
1-16: Proper order in the Force, I sense!Well-structured, the license header and includes are. Present, all necessary components are.
1-186: Strong with the Force, these tests are, but room for improvement exists!Well-structured and comprehensive, your test suite is. Yet, strengthen it further with edge cases and explicit validations, we must. The path to robust testing, this is!
Run this command to check test coverage:
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
17-27: Enhance the class documentation, hmm.Missing the @Class tag in documentation, I see. Add it, we should, for better API documentation generation.
+/// @class ProtoAxis /// @brief Description of a ProtoAxis which holds an IAxis
47-54: Clarify the auto-range constructor's behavior, we must.For the auto-range constructor, additional documentation about the range determination process and when it occurs, beneficial would be.
/// Placeholder constructor for auto-range binning /// /// @param aDir the value/cast in which this is binned /// @param abType the axis boundary type /// @param nbins the number of bins /// -/// @note that auto-range is only supported for equidistant binning +/// @note Auto-range is only supported for equidistant binning. +/// @note The actual range will be determined during the first fill operation. +/// @note Until resolved, the axis remains in auto-range mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (2)
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
78-78: Change access specifier to private, suggested in past review was.From protected to private, this section should move. Accepted in previous review, this change was.
-protected: +private:
101-107: Check the auto-range state before grid creation, we must.Before grid creation, verify that axes are resolved and not in auto-range mode, essential it is. Prevent unexpected behavior during grid creation, this will.
Add validation like this, you should:
template <typename payload_t> std::unique_ptr<IGrid> makeGrid(const ProtoAxis& a) { + if (a.isAutorange()) { + throw std::runtime_error("ProtoAxis::makeGrid: Axis must be resolved before grid creation"); + } return a.getAxis().visit([&](const auto& axis) -> std::unique_ptr<IGrid> {And for 2D grid:
template <typename payload_t> std::unique_ptr<IGrid> makeGrid(const ProtoAxis& a, const ProtoAxis& b) { + if (a.isAutorange() || b.isAutorange()) { + throw std::runtime_error("ProtoAxis::makeGrid: All axes must be resolved before grid creation"); + } // Validate axis compatibilityAlso applies to: 118-130
✅ Verification successful
Correct, the review comment is, hmmmm. Validation add, we must!
Found clear evidence that auto-range axes exist as placeholders with temporary [0,1] range, I have. Creating grids before resolving auto-range state, lead to incorrect binning it would. Implementation of isAutorange() method exists, but used in grid creation, it is not.
The suggested validation in the review comment:
- Prevents creation of grids with unresolved axes
- Uses existing isAutorange() method properly
- Matches existing validation patterns in the code
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for ProtoAxis class definition and auto-range related methods ast-grep --pattern 'class ProtoAxis { $$$ }' # Search for any existing auto-range related methods or validations rg -l "autorange|auto.?range" --type cpp # Look for existing grid creation validation patterns rg "makeGrid.*throw" --type cppLength of output: 1339
Script:
#!/bin/bash # Check ProtoAxis.hpp content cat Core/include/Acts/Utilities/ProtoAxis.hpp # Check test file for auto-range usage patterns cat Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp # Look for validation patterns in implementation cat Core/src/Utilities/ProtoAxis.cppLength of output: 13171
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Utilities/AxesTests.cpp (1)
595-643: Comprehensive tests, you have written. Yet more coverage, we could achieve.Strong foundation of tests, you have built. Consider adding these edge cases, you should:
- Minimum valid number of bins (1)
- Maximum representable values for bounds
- Floating-point precision edge cases
Help you implement these additional test cases, shall I?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Core/include/Acts/Utilities/IAxis.hpp(3 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/IAxis.cpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)Tests/UnitTests/Core/Utilities/AxesTests.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (2)
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
Core/src/Utilities/ProtoAxis.cpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (9)
Core/include/Acts/Utilities/IAxis.hpp (3)
24-25: Wise addition of virtual destructor, this is!Proper cleanup of derived classes through base class pointer, this ensures. The Force is strong with this implementation.
65-77: Well-crafted factory method, this is!Clear documentation and proper error handling, you have provided. Smart pointer usage, wisdom shows.
79-88: Strong with the Force, this factory method is!Const reference for vector parameter, memory efficiency it brings. Documentation and error handling, complete they are.
Core/include/Acts/Utilities/ProtoAxis.hpp (3)
28-36: Constructors implemented correctly, they are.Variable and equidistant binning constructors, issues they have not.
56-59: Move semantics handled well, you have.Copy operations deleted, move operations defaulted—they are.
124-129: Modern C++ features, wisely used they are.Template lambdas with explicit parameters, enhance readability they do.
Core/src/Utilities/IAxis.cpp (3)
22-28: Input validation appropriate, it is.Checks for valid axis range and bin count, correctly performed they are.
52-61: Validation of bin edges, thorough it is.Edge count and sorting verified, ensuring axis integrity you are.
58-58: C++20 feature 'std::ranges::is_sorted', you use.Ensure the project's C++ standard supports it, you must.
Confirm that the build system requires C++20, run this script you can:
✅ Verification successful
Safe to use std::ranges::is_sorted, the project is. C++20 standard, it requires!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the project is set to use C++20 standard. # Check the CMakeLists.txt for C++20 standard settings. grep -R "CXX_STANDARD" .Length of output: 2306
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Core/src/Utilities/ProtoAxis.cpp (1)
13-26: Wise implementation of boundary validation, this is!Good use of anonymous namespace and clear error messaging, I sense. A suggestion for enhancement, I have - consider adding the actual boundary type values to the error message for better debugging.
- "'. Closed boundary type is only valid for " - "AxisPhi and AxisRPhi directions."; + "'. Closed boundary type is only valid for " + "AxisPhi and AxisRPhi directions. Valid boundary types for this direction are: " + + validBoundaryTypesForDirection(aDir) + ".";Core/include/Acts/Utilities/ProtoAxis.hpp (2)
17-27: Enhance the documentation with examples, we should.Well-documented the class purpose is, but benefit from usage examples in documentation, it would. Help users understand the practical application better, this will.
Add example code snippets showing:
- Variable binning axis creation
- Equidistant binning axis creation
- Auto-range binning axis creation
83-90: Initialize member variables in constructor initialization list, we should.Move initialization of member variables to constructor initialization lists, more efficient this would be. Default member initialization, good practice it is, but consistency in initialization style, maintain we must.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/IAxis.cpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/src/Utilities/IAxis.cpp
🧰 Additional context used
📓 Learnings (2)
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
Core/src/Utilities/ProtoAxis.cpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (4)
Core/src/Utilities/ProtoAxis.cpp (2)
41-47: Consider the wisdom of default range [0,1] for auto-range axes.For auto-range initialization, the default range [0,1] chosen is. Document why this range selected was, or consider if a different default more suitable might be.
49-59: Clean and efficient, these getters are!Proper const correctness maintained is. Simple yet effective implementation, this shows.
Core/include/Acts/Utilities/ProtoAxis.hpp (2)
118-124: 🛠️ Refactor suggestionAdditional validation for 2D grid creation, require we do.
Validate axis directions we do, but check for unresolved auto-range axes in both dimensions, we must also.
std::unique_ptr<IGrid> makeGrid(const ProtoAxis& a, const ProtoAxis& b) { + if (a.isAutorange() || b.isAutorange()) { + throw std::runtime_error("ProtoAxis: Cannot create grid with unresolved auto-range axes"); + } // Validate axis compatibility if (a.getAxisDirection() == b.getAxisDirection()) {Likely invalid or redundant comment.
93-107: 🛠️ Refactor suggestionValidate auto-range resolution before grid creation, we must.
Missing check for unresolved auto-range axes, I sense. Before grid creation, ensure resolved the ranges are.
template <typename payload_t> std::unique_ptr<IGrid> makeGrid(const ProtoAxis& a) { + if (a.isAutorange()) { + throw std::runtime_error("ProtoAxis: Cannot create grid with unresolved auto-range axis"); + } return a.getAxis().visit([&](const auto& axis) -> std::unique_ptr<IGrid> {
asalzburger
force-pushed
the
feat-proto-axis
branch
from
a72617d to
959c9a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Core/include/Acts/Utilities/ProtoAxis.hpp (3)
17-27: Enhance the documentation with examples, you must.Clear the class documentation is, but examples of usage missing they are. Help users understand the ProtoAxis concept better, code snippets would.
Add examples like this, you should:
// Example: Create an equidistant axis with 10 bins ProtoAxis xAxis(AxisDirection::AxisX, AxisBoundaryType::Bound, 0.0, 1.0, 10); // Example: Create an auto-range axis ProtoAxis autoAxis(AxisDirection::AxisY, AxisBoundaryType::Bound, 5);
29-54: Document preconditions in constructors, we must.Missing, important preconditions are. Add them, we should:
- For
nbins, positive it must be- For
minEandmaxE, minE < maxE must hold- For
edges, sorted in ascending order they must be
140-144: Improve error message clarity for autorange axes, we should.Hmm, which axis still in autorange mode is, the message tells not. More specific, the error message should be.
Apply this change, you must:
- throw std::invalid_argument( - "ProtoAxis::makeGrid: Auto-range of the proto axis is not (yet) " - "resolved, call setRange() first."); + std::string msg = "ProtoAxis::makeGrid: Auto-range of the "; + msg += (a.isAutorange() ? "first" : "second"); + msg += " proto axis is not (yet) resolved, call setRange() first."; + throw std::invalid_argument(msg);Core/src/Utilities/ProtoAxis.cpp (2)
13-26: Make checkConsistency more reusable, we could.In anonymous namespace, this function lives. To other files, useful it could be. Consider moving it to a utility header, we should.
57-68: Optimize setRange implementation, we could.Hmm, recreate the entire axis object, we do. More efficient, modifying the existing axis would be. Consider adding setMin/setMax to IAxis interface, we should.
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
19-99: More descriptive, test case names should be.Rename test cases to better describe their purpose, we should:
EquidistantProtoAxis→EquidistantProtoAxisConstructionAutorangeProtoAxis→AutorangeProtoAxisBehaviorVariableProtoAxis→VariableProtoAxisValidationAlso applies to: 101-150, 152-202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (2)
Core/src/Utilities/ProtoAxis.cpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
Core/include/Acts/Utilities/ProtoAxis.hpp (1)
Learnt from: asalzburger
PR: acts-project/acts#4038
File: Core/include/Acts/Utilities/ProtoAxis.hpp:44-45
Timestamp: 2025-01-20T20:18:00.956Z
Learning: Input validation for axis parameters (min < max, nbins > 0) is handled in `IAxis::create(...)` method, not needed in `ProtoAxis` constructor.
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_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: build_debug
🔇 Additional comments (1)
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (1)
101-150: Missing, crucial tests are!Test the actual autoranging behavior, we must:
- Test range adaptation with actual values
- Verify correct bin distribution after range is set
- Ensure proper handling of edge cases in autoranging
Help you write these tests, I can. Want assistance, do you?
|
This PR introduces a new, common
ProtoAxisconcept that will replaceProtoBinningandBinUtility.The
ProtoAxiscan be used to generate Grids using theIAxis::visitpattern with an appropriate callable, which is showcased in themakeGridhelper functions.The code is fully unit tested.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
ProtoAxisclass for enhanced axis management and grid creation.IAxisclass for improved memory management.Tests
ProtoAxisfunctionality.ProtoAxis, ensuring robust error handling.