test: Bounds regression tests #4001
Conversation
WalkthroughHmm, changes to boundary tolerance and surface bounds testing, we have. In the Core library, modifications to Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Core/src/Surfaces/BoundaryTolerance.cpp (1)
Line range hint
173-202: Chi2 and Euclidean logic robust, watch negative values you must.
Critical correctness needed, yes.
🧹 Nitpick comments (13)
Tests/UnitTests/Core/Surfaces/CMakeLists.txt (1)
Line range hint
1-31: Organize tests by category, we should.Group related tests together, a Jedi's path to clarity is. Suggest organizing tests into logical categories, I do:
# Boundary and Bounds Tests add_unittest(BoundaryTolerance BoundaryToleranceTests.cpp) add_unittest(BoundsRegression BoundsRegressionTests.cpp) add_unittest(AnnulusBounds AnnulusBoundsTests.cpp) # ... other bounds tests # Surface Tests add_unittest(ConeSurface ConeSurfaceTests.cpp) add_unittest(CylinderSurface CylinderSurfaceTests.cpp) # ... other surface tests # Helper Tests add_unittest(IntersectionHelper2D IntersectionHelper2DTests.cpp) add_unittest(VerticesHelper VerticesHelperTests.cpp) add_unittest(AlignmentHelper AlignmentHelperTests.cpp)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (1)
129-134: “Check sign” the comment says, a path unknown it is.
Assistance provide, I can, yes.Core/src/Surfaces/BoundaryTolerance.cpp (1)
115-115: Assert for unsupported types, a cautionary measure it is.
Defensive coding, value it brings.Core/src/Surfaces/AnnulusBounds.cpp (2)
242-253: Closest point computation, thorough indeed.
Large overhead it might have, carefully watch.
436-436: Logic error thrown, next steps or expansions needed?
Assistance for completion, offer I shall.Tests/UnitTests/Core/Surfaces/BoundsRegressionTests.cpp (3)
73-174: Matrix-based approach, interesting indeed.
Performance in larger grids, watch carefully you must.
358-371: Reference matrix usage, consistent it is.
Potential for data storage improvement, yes.
373-478: Comparisons to textual reference, thorough indeed.
Future expansions for shapes, wise would be.Core/include/Acts/Surfaces/BoundaryTolerance.hpp (1)
70-74: More specific error messages, suggest I do.Hmm, more descriptive the error messages should be, including the actual values that caused the error.
Apply this change, you should:
- "AbsoluteBound: Tolerance must be non-negative"); + "AbsoluteBound: Tolerances must be non-negative, but got tolerance0=" + + std::to_string(tolerance0) + ", tolerance1=" + std::to_string(tolerance1));- "AbsoluteCartesian: Tolerance must be non-negative"); + "AbsoluteCartesian: Tolerances must be non-negative, but got tolerance0=" + + std::to_string(tolerance0) + ", tolerance1=" + std::to_string(tolerance1));Also applies to: 85-89
Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (1)
144-243: Documentation for test cases, improve we should.Good the test coverage is, but clearer documentation we need. Each test section's purpose and expectations, document we must.
Add documentation blocks like this, you should:
+ // Test positive absolute euclidean tolerance { auto tolerance = BoundaryTolerance::AbsoluteEuclidean(1); // ... existing tests ... } + // Test negative absolute euclidean tolerance for boundary shrinking { auto tolerance = BoundaryTolerance::AbsoluteEuclidean(-1); // ... existing tests ... } + // Test positive chi-squared tolerance with identity weight matrix { auto tolerance = BoundaryTolerance::Chi2Bound(SquareMatrix2::Identity(), 0.1); // ... existing tests ... }Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (3)
30-108: Refactor test helpers, we should.Repeated patterns in tests, I see. Extract common test logic into helper functions, we must.
Create helper functions like this:
template <typename T> void checkToleranceMode(const T& tolerance, BoundaryTolerance::ToleranceMode expected) { BOOST_CHECK(BoundaryTolerance{tolerance}.toleranceMode() == expected); } template <typename T> void checkNegativeToleranceThrows(std::function<T(double,double)> constructor) { BOOST_CHECK_THROW(constructor(-1.0, 2.0), std::invalid_argument); BOOST_CHECK_THROW(constructor(1.0, -2.0), std::invalid_argument); }
289-335: Name test points better, we must.Clear the test structure is, but meaning of test points, obscure it remains.
Add descriptive names for test points:
- BOOST_CHECK(!check(tolerance, {2.8, 2})); + Vector2 nearUpperBoundary{2.8, 2}; + BOOST_CHECK(!check(tolerance, nearUpperBoundary)); - BOOST_CHECK(!check(tolerance, {3.1, 2})); + Vector2 outsideUpperBoundary{3.1, 2}; + BOOST_CHECK(!check(tolerance, outsideUpperBoundary));
337-385: Visual documentation, add we should.Clear the test cases are, but visual representation would help understanding, it would.
Add ASCII art to document the test points:
+ // Test points for trapezoid: + // + // (1,3)-----(3,3) + // / \ + // / \ + // / \ + // (1.5,1)----------(2.5,1) + // Vector2 vertices[] = {{1.5, 1}, {2.5, 1}, {3, 3}, {1, 3}};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp(4 hunks)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(4 hunks)Core/src/Surfaces/AnnulusBounds.cpp(2 hunks)Core/src/Surfaces/BoundaryTolerance.cpp(3 hunks)Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp(2 hunks)Tests/UnitTests/Core/Surfaces/BoundsRegressionTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Tests/UnitTests/Core/Surfaces/BoundsRegressionTests.cpp
[error] 175-175: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (16)
Tests/UnitTests/Core/Surfaces/CMakeLists.txt (1)
31-31: Hmmmm, good this addition is, young padawan!
Wise decision to add regression tests for bounds, I sense. Strong with the Force, this change is.
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (3)
32-47: Stable the new tolerance usage is.
Implementation consistent, yes.
73-77: Extend mode logic, correct it looks.
Negative tolerances, test carefully you must.
92-112: Clarity improved, handle boundary checks gracefully you do.
Minimal overhead, the code suggests.
Core/src/Surfaces/BoundaryTolerance.cpp (3)
63-70: Shift from boolean to modes, wise you are.
Better clarity it grants.
75-79: Zero tolerance fallback to None, correct that is.
Neat boundary condition logic, yes.
84-111: Multiple tolerance variants well-handled.
Coverage for all types, thorough it seems.
Core/src/Surfaces/AnnulusBounds.cpp (3)
Line range hint 210-236: Integrate BoundaryTolerance well, you do.
Seamless the approach is, hmm.
385-403: Chi2Bound checks in place, well-defined.
Additional tests for edge scenarios, wise would be.
404-433: Absolute Euclidean checks, correct they appear.
Fitting seamlessly with negative tolerance logic, yes.
Tests/UnitTests/Core/Surfaces/BoundsRegressionTests.cpp (4)
1-36: New test harness, good variety it introduces.
Methodical coverage of shapes, nice it is.
37-66: Bounding box generation, margin approach wise.
Corner cases well-handled, see I do.
177-273: Rectangle and trapezoid references, verified they are.
Confidence in comparisons, yes.
480-481: Test suite concludes, heed not the macro confusion.
Macros well-known in Boost they are.
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (2)
111-115: Strong with the Force, this enum is.
Clear and concise, the ToleranceMode enum class is. Good practices it follows, using enum class for type safety.
151-151: Breaking change, this is. Verify all callers, we must.
Changed from boolean to enum return type, this method has. Impact on existing code, we must check.
✅ Verification successful
The previous search for hasTolerance() yielded no results, suggesting the change might have already been propagated. Let me search for the new method and class usage to verify the current state of integration.
Safe, this change is. No impact on existing code, I sense.
Through the Force, I have observed that hasTolerance() method no longer exists in the codebase, and all usages already utilize the new toleranceMode() API. The change appears to be part of a completed transition, hmm.
The search results show:
- No remaining calls to
hasTolerance() - Extensive test coverage for the new
toleranceMode()API - All code using the new enum-based approach consistently
Complete, this transition already is. No further changes needed, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to hasTolerance() that need updating
rg -l "hasTolerance\(\)" | while read -r file; do
if [[ "$file" =~ .*\.(cpp|hpp|h)$ ]]; then
echo "=== $file ==="
rg -A 2 "hasTolerance\(\)" "$file"
fi
done
Length of output: 41
Script:
#!/bin/bash
# Search for all usages of toleranceMode() in the codebase
rg -A 2 "toleranceMode\(\)"
# Also search for BoundaryTolerance class usage
rg -A 2 "BoundaryTolerance"
Length of output: 66038
|
@paulgessinger can you rebase and change the title? |
paulgessinger
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (1)
Line range hint
134-138: Documentation for ToleranceMode enum, improve we must.Clear the purpose is, but documentation for each mode value, missing it is. Add comments to explain when each mode is used and its impact on boundary calculations, we should.
Apply this enhancement, you should:
enum class ToleranceMode { - Extend, // Extend the boundary - None, // No tolerance - Shrink // Shrink the boundary + Extend, /// Extends the boundary outward by the tolerance value + None, /// No modification to the boundary, exact checks performed + Shrink /// Shrinks the boundary inward by the tolerance value };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp(1 hunks)
🔇 Additional comments (2)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (2)
90-93: Wisdom in validation, I sense.
Balance in the tolerances, there must be. Either both zero or both non-zero, they should be. Well-crafted error handling, this is.
Line range hint 134-138: Verify the impact of ToleranceMode changes, we must.
From hasTolerance() to toleranceMode(), the path leads. Ensure all callers updated are, we must.
✅ Verification successful
Hmmmm, clear the path forward is. No traces of 'hasTolerance()' remain, I sense.
Through the Force, observed I have:
- All usages of
toleranceMode()properly aligned are - No lingering
hasTolerance()calls exist in the codebase - Test cases updated correctly they are, showing proper usage of new API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of hasTolerance()
rg "hasTolerance\(\)"
# Search for new toleranceMode() usage
rg "toleranceMode\(\)"
Length of output: 1916
|
acts-project-service
added
the
Breaks Athena build
This PR adds regression tests based on a grid of sample points. The bounds on a surface are checked, and the result is compared to a reference matrix that is parsed from a string. In case of a mismatch a diff like this is printed:
We'll probably extend this to more shapes and tolerance scenarios. This is a proof of concept only.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores