refactor: GeometryIdentifier checks value range #4094
Conversation
WalkthroughA new static method Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant GI as GeometryIdentifier
C->>GI: setBits(mask, id)
GI->>GI: getMaxValue(mask)
alt id within valid range
GI-->>C: Bits set successfully
else id exceeds max value
GI-->>C: Throw std::invalid_argument
end
Suggested labels
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 (
|
paulgessinger
force-pushed
the
refactor/geo-id-checked
branch
from
3fd9454 to
5673f81
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (1)
130-140:⚠️ Potential issueMissing headers, I sense. Include them, you must!
Required headers for exception handling and string operations, missing they are. Add them, you must:
+#include <stdexcept> +#include <string>Prevent compilation errors, this will. Strong in error handling, your code is, but headers it needs!
🧰 Tools
🪛 GitHub Actions: Analysis
[error] 132-132: 'invalid_argument' is not a member of 'std'. Did you forget to '#include '?
[error] 133-133: 'to_string' is not a member of 'std'. Did you forget to '#include '?
[error] 134-134: 'to_string' is not a member of 'std'. Did you forget to '#include '?
🧹 Nitpick comments (2)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (1)
95-106: Document the maximum values, we should!In comments, the maximum values shown are. But in class documentation, mention them we should. Help future Padawans understand the limits, it will.
Add to class documentation, you should:
/// - Approach surfaces (for a layer) /// - Sensitive surfaces (confined to a layer, also called modules) +/// +/// Maximum values for each component: +/// - Volume: 255 (8 bits) +/// - Boundary: 255 (8 bits) +/// - Layer: 4095 (12 bits) +/// - Approach: 255 (8 bits) +/// - Sensitive: 1048575 (20 bits) +/// - Extra: 255 (8 bits)Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp (1)
48-59: Well tested, these changes are!Comprehensive test coverage, you have achieved. All components for overflow, you check. Exception handling, properly verified it is.
Suggest one improvement, I do. Add test case for boundary values, we should:
+ // verify maximum values are accepted + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setVolume(volumeMax)); + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setBoundary(boundaryMax)); + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setLayer(layerMax)); + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setApproach(approachMax)); + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setSensitive(sensitiveMax)); + BOOST_CHECK_NO_THROW(GeometryIdentifier(ref).setExtra(extraMax));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Geometry/GeometryIdentifier.hpp(1 hunks)Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Analysis
Core/include/Acts/Geometry/GeometryIdentifier.hpp
[error] 132-132: 'invalid_argument' is not a member of 'std'. Did you forget to '#include '?
[error] 133-133: 'to_string' is not a member of 'std'. Did you forget to '#include '?
[error] 134-134: 'to_string' is not a member of 'std'. Did you forget to '#include '?
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
🔇 Additional comments (1)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (1)
121-123: Clean and efficient, this implementation is!Wisely implemented, the
getMaxValuemethod is. Reuse existingextractShiftfunction it does, and maximum value correctly calculate it does.
|
@benjaminhuth I don't think so. I pushed a commit adding a missing include. Can you approve again? |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (1)
97-107: 💡 Verification agent🧩 Analysis chain
Verify the wisdom of our masks, we must.
Test all bit masks with maximum values, essential it is.
🏁 Script executed:
#!/bin/bash # Description: Generate test cases for all masks maximum values echo "Generating test cases for maximum values:" for mask in "kVolumeMask" "kBoundaryMask" "kLayerMask" "kApproachMask" "kSensitiveMask" "kExtraMask"; do rg --type cpp "static constexpr.*$mask.*=.*0x([0-9a-f]+)" -o | \ awk '{print "Test case for " $1 ": value = 0x" $NF}' doneLength of output: 2065
Verify the profound wisdom of our masks, we must – with a slight refinement, yes.
The bit masks defined in
Core/include/Acts/Geometry/GeometryIdentifier.hpphave been exercised with maximum values, and their outputs confirm their intended sizes. Hmmm, however, a minor quirk observed there is: the printed output from the test script shows a redundant "0x" prefix (for example, printing0x0x00ff000000000000). Adjust the test script’s output formatting—perhaps by removing the additional "0x" in the AWK print command—so clarity, we ensure.
- Confirmed, the masks produce their maximum values as intended.
- Suggested, clean the output from the test script to remove the duplicate "0x" prefix.
- Test script remains largely the same, serving its purpose well.
May the Force guide you in polishing this detail.
🧹 Nitpick comments (2)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (2)
122-124: Strong with the Force, this method is. Yet documentation, it lacks.Clear and concise implementation, but document its purpose and return value, we must.
Add documentation like this, you should:
+ /// Calculate the maximum allowed value for a given bit mask + /// @param mask The bit mask to calculate maximum value for + /// @return The maximum value that can be stored in the masked bits constexpr static Value getMaxValue(Value mask) {
132-136: Protect against the dark side of overflow, this validation does.Clear error message, it provides. Yet discuss alternative approaches to exception throwing, we should.
Consider these alternatives, you might:
- Return Result instead of throwing
- Clamp value to maximum instead of throwing
- Add a validation-only method for upstream checks
Your thoughts on these approaches, share with us you must, @benjaminhuth and @andiwand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Geometry/GeometryIdentifier.hpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: macos
🔇 Additional comments (1)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (1)
14-14: Wise addition of error handling capabilities, this is.The force of std::invalid_argument, properly included it now is.
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
paulgessinger
force-pushed
the
refactor/geo-id-checked
branch
from
2764d27 to
dec1164
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp (1)
48-59: Test the withXXX methods also, we must!From past learnings, wisdom we gain. Both setXXX and withXXX methods, tested they should be. Compatibility with both mutable and immutable APIs, ensure we must.
Add these tests, you should:
+ BOOST_CHECK_THROW(GeometryIdentifier(ref).withVolume(volumeMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).withBoundary(boundaryMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).withLayer(layerMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).withApproach(approachMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).withSensitive(sensitiveMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).withExtra(extraMax + 1), + std::invalid_argument);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Geometry/GeometryIdentifier.hpp(1 hunks)Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#4095
File: Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp:63-72
Timestamp: 2025-02-17T14:31:10.699Z
Learning: In GeometryIdentifier tests, both `setXXX` and `withXXX` methods should be tested to ensure compatibility with both mutable and immutable APIs.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: merge-sentinel
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (3)
Core/include/Acts/Geometry/GeometryIdentifier.hpp (2)
192-194: Approve this implementation, I do!Simple yet powerful, this method is. Maximum value from mask, correctly it calculates. Clean and efficient, the implementation remains.
201-212: Wise decision to add value range checks, this is!Prevent overflow issues, this change will. Clear error messages, it provides. The Force, strong with this implementation, it is.
Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp (1)
48-59: Approve these tests, I do!Well-crafted tests, these are. Each component's boundaries, they verify. Exception handling, thoroughly tested it is.
|
acts-project-service
added
the
Fails Athena tests
Surface the maximum values for the `GeometryIdentifier` components. Blocked by: - #4094
I've run into overflowing ID values one too many times now. This PR proposes making the value setters check the value to be assigned to be within the valid range.
I'm currently throwing an exception, which we can debate if that's the best way to handle failure.
--- END COMMIT MESSAGE ---
Thoughts @benjaminhuth @andiwand?
Summary by CodeRabbit