feat: Support negative tolerance for euclidean + chi2 tests #4000
Conversation
WalkthroughHmm, a journey through boundary tolerances, this pull request is. Enhancements 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: 0
🧹 Nitpick comments (7)
Core/src/Surfaces/AnnulusBounds.cpp (2)
209-211: Covariance transform commentary, helpful it is.
Expand or refine docs, consider you might.
371-400: AbsoluteEuclidean tolerance, large block of logic.
Split into smaller methods, consider you should.Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (5)
152-154: Remove debug print statements, you must.Debug print statements in test cases, left behind they were. In production code, belong they do not.
- std::cout << "[" << pointAverage[0] << ", " << pointAverage[1] << "]," - << std::endl;
249-265: Remove commented code blocks, we shall.Commented test cases, lurking in shadows they are. Clean code, maintain we must. Either restore or remove them, you should.
- // Test points near radial boundaries - // BOOST_CHECK(!check(tolerance, {minRadius - 0.2, 0})); - // BOOST_CHECK(!check(tolerance, {minRadius + 0.2, 0})); - // BOOST_CHECK(check(tolerance, {minRadius + 0.4, 0})); - // BOOST_CHECK(!check(tolerance, {maxRadius - 0.2, 0})); - // BOOST_CHECK(!check(tolerance, {maxRadius + 0.2, 0})); - // BOOST_CHECK(check(tolerance, {maxRadius - 0.4, 0})); - - // // Test points near angular boundaries - // BOOST_CHECK(!check(tolerance, {(minRadius + maxRadius) / 2, minPhi - - // 0.2})); BOOST_CHECK(!check(tolerance, {(minRadius + maxRadius) / 2, - // minPhi + 0.2})); BOOST_CHECK(check(tolerance, {(minRadius + maxRadius) / - // 2, minPhi + 0.4})); BOOST_CHECK(!check(tolerance, {(minRadius + - // maxRadius) / 2, maxPhi - 0.2})); BOOST_CHECK(!check(tolerance, - // {(minRadius + maxRadius) / 2, maxPhi + 0.2})); - // BOOST_CHECK(check(tolerance, {(minRadius + maxRadius) / 2, maxPhi - - // 0.4}));
149-155: Improve lambda function clarity, we must.The lambda function
check, cryptic it appears. Documentation and meaningful variable names, add we should.- auto check = [&](const BoundaryTolerance& tolerance, const Vector2& point) { - Vector2 pointAverage(point[0], phiAverage + point[1]); - std::cout << "[" << pointAverage[0] << ", " << pointAverage[1] << "]," - << std::endl; - return aBounds.inside(pointAverage, tolerance); - }; + // Helper function to check if a point is inside the annulus bounds + // point[0]: radial coordinate + // point[1]: phi offset from average phi + auto isPointInsideBounds = [&](const BoundaryTolerance& tolerance, + const Vector2& point) { + Vector2 testPoint(point[0], phiAverage + point[1]); + return aBounds.inside(testPoint, tolerance); + };
246-247: Remove debugging artifacts, we shall.Commented-out function calls
mc(tolerance), purpose unclear they are. Remove such artifacts, maintain clean tests we must.- // mc(tolerance); - // mc(BoundaryTolerance::Chi2Bound(SquareMatrix2::Identity(), 0.1));
144-297: Test documentation, improve we should.Though comprehensive the tests are, documentation sparse it remains. Add comments explaining test strategy and expected behavior for each tolerance type.
+/// Test case for boundary checks with negative tolerances +/// Tests both Euclidean and Chi2 boundary tolerances with positive and negative values +/// - Positive tolerance: extends the boundary outward +/// - Negative tolerance: contracts the boundary inward +/// Tests points near both radial and axial boundaries at various distances BOOST_AUTO_TEST_CASE(AnnulusBoundsNegativeTolerance) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp(4 hunks)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(5 hunks)Core/src/Surfaces/AnnulusBounds.cpp(2 hunks)Core/src/Surfaces/BoundaryTolerance.cpp(4 hunks)Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp(2 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)
🔇 Additional comments (26)
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (6)
11-14: Include statements, strong start they are.
No issues do I sense.
33-48: Tolerance mode usage, clear it is.
Implementation well-aligned with design, hmmm.
74-78: Extend vs. shrink logic, harmonious it appears.
Covers both scenarios, yes.
93-110: Inside polygon logic, consistent with ToleranceMode it is.
Solid, no concerns do I have.
112-113: Extend mode short-circuit, efficient this is.
Return quickly if already inside, hmm.
130-135: Distance sign flipped, careful we must be.
Verify correctness for Shrink mode, you should.
Core/src/Surfaces/BoundaryTolerance.cpp (4)
13-13: New includes, yes.
Valuable for math and error handling, hmmm.
65-141: Comprehensive toleranceMode method, well-structured it is.
Yet cautious with sign checks, be.
199-204: Negative maxChi2 usage, interesting approach.
Test thoroughly, confirm correctness you must.
Line range hint 220-226: Handling negative Euclidean tolerance, a corner case that is.
Ensure no unintended side effects, you should.
Core/src/Surfaces/AnnulusBounds.cpp (8)
177-177: Using ToleranceMode enum, clarity you have.
No problem do I see.
192-192: Strict inside check, wise to confirm.
Matches typical usage, it does.
194-194: Retrieving ToleranceMode, essential step it is.
No issues.
195-198: Mode None, apply no tolerance indeed.
Logic straightforward, yes.
201-203: Extend mode plus insideStrict, quick exit you have.
Efficiency strong in this path.
219-220: Lambda for distance checking, modular solution it provides.
Implementation neat, yes.
352-370: Chi2Bound logic, robust approach it seems.
Negative maxChi2, tricky corner case that is.
403-403: Throwing not implemented, fallback do we need?
Investigate or handle gracefully, you could.
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (4)
73-78: Negative tolerance forbidden, correct this is.
Exception usage, wise choice it may be.
88-93: Cartesian tolerance also non-negative, consistent logic we have.
No conflicts do I foresee.
114-119: Enum ToleranceMode introduced, clarity it bestows.
Extend, None, Shrink – well named, hmm.
154-154: Renaming hasTolerance to toleranceMode, more precise it is.
All references, update carefully you must.
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (3)
30-86: Thorough constructor tests, these are. Approve them, I do.
All tolerance types tested they are. Both positive and negative cases, covered they have been. Exception handling, verified it is.
267-313: Well-crafted rectangular boundary tests with negative tolerance, these are.
Comprehensive test coverage for negative tolerances in rectangular boundaries, achieved it has. Both Euclidean and Chi2 tolerances, tested they are.
315-363: Trapezoidal boundary tests with negative tolerance, masterfully done they are.
Edge cases and corner cases, thoroughly tested they have been. Both positive and negative tolerances, validated they are.
Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (1)
144-297: Well structured test cases, I sense.
Strong with the Force, these test cases are. Comprehensive coverage of boundary conditions, they provide:
- Positive and negative Euclidean tolerances
- Positive and negative Chi2 tolerances
- Radial and axial boundary points
- Various distances from boundaries
Clear progression from simple to complex cases, maintained it is.
paulgessinger
force-pushed
the
boundary-tolerance-negative
branch
from
22a591a to
b4817ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (1)
70-75: Improve error message for AbsoluteBound constructor, hmm.More descriptive, the error message should be. Help developers understand which parameter was negative, we must.
- "AbsoluteBound: Tolerance must be non-negative"); + "AbsoluteBound: Tolerances must be non-negative. Got tolerance0=" + + std::to_string(tolerance0_) + ", tolerance1=" + + std::to_string(tolerance1_));Core/src/Surfaces/BoundaryTolerance.cpp (2)
79-82: Remove debug comments, we must.Left behind, these debug statements were. Clean code, maintain we should.
- // std::cout << absoluteBound->tolerance0 << " " - // << std::copysign(1., absoluteBound->tolerance0) << std::endl; - // std::cout << absoluteBound->tolerance1 << " " - // << std::copysign(1., absoluteBound->tolerance1) << std::endl;
197-202: Document the magic number 2, we should.Mysterious, the number 2 is. Its meaning in chi-square calculations, explain we must.
+ // For 2D vectors, chi-square distribution has 2 degrees of freedom + // Hence we multiply maxChi2 by 2 for proper scaling double chi2 = distance.transpose() * chi2Bound->weight * distance; if (chi2Bound->maxChi2 < 0) { return chi2 > 2 * std::abs(chi2Bound->maxChi2); } else { return chi2 <= 2 * chi2Bound->maxChi2; }Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (1)
30-86: Strong with tests, this code is! Yet stronger, it can become.Good coverage of constructors, we see. But missing edge cases, there are:
- Zero tolerances, test we should
- Infinity values, handle we must
- NaN values, reject we shall
Additional test cases, generate I can. Your approval, shall I seek?
Core/src/Surfaces/AnnulusBounds.cpp (2)
385-402: Extract magic number into constant, we should!Square the maxChi2 multiple times, you do. A constant define, you should, for clarity and maintenance.
+ // Square of maxChi2 used for boundary comparisons + const double squaredMaxChi2 = boundaryToleranceChi2.maxChi2 * + boundaryToleranceChi2.maxChi2; if (mode == Extend) { - return minDist < boundaryToleranceChi2.maxChi2 * boundaryToleranceChi2.maxChi2; + return minDist < squaredMaxChi2; } else if (mode == Shrink) { - return minDist > boundaryToleranceChi2.maxChi2 * boundaryToleranceChi2.maxChi2 && + return minDist > squaredMaxChi2 && insideStrict; }
403-433: Similar wisdom applies here, it does!Square the tolerance multiple times, you do. A constant define, you should, as with Chi2 handling.
+ const double squaredTolerance = + boundaryToleranceAbsoluteEuclidean.tolerance * + boundaryToleranceAbsoluteEuclidean.tolerance; if (mode == Extend) { - return cartesianDistance.squaredNorm() < - boundaryToleranceAbsoluteEuclidean.tolerance * - boundaryToleranceAbsoluteEuclidean.tolerance; + return cartesianDistance.squaredNorm() < squaredTolerance; } else if (mode == Shrink) { - return cartesianDistance.squaredNorm() > - boundaryToleranceAbsoluteEuclidean.tolerance * - boundaryToleranceAbsoluteEuclidean.tolerance && + return cartesianDistance.squaredNorm() > squaredTolerance && insideStrict; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp(4 hunks)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(5 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp
- Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp
🧰 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)
🔇 Additional comments (4)
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (1)
111-115: Good, the enum design is! Approved, it shall be.
Clear and concise, the enum values are. Well-scoped as enum class, preventing namespace pollution, it does.
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (1)
267-363: Impressive, these boundary tests are! Approved, they shall be.
Thorough testing of negative tolerances, achieved it has:
- Corner cases, covered they are
- Edge conditions, tested they be
- Regular points, verified they remain
Core/src/Surfaces/AnnulusBounds.cpp (2)
Line range hint 210-236: Approve the tolerance handling, I do!
Clean and efficient, the early returns are. Clear separation of concerns, you have achieved. Good practices, you follow!
252-383: Verify numerical stability, we must!
Complex mathematical transformations, I see. Though well-documented they are, edge cases we must consider. Near-zero denominators in matrix calculations, dangerous they can be.
✅ Verification successful
Numerical stability concerns, valid they are not!
Examined the code thoroughly, I have. Safe from the Dark Side of numerical instability, this implementation is. These reasons, I shall share:
- Protection against division by zero, the code has. Variable
Ain denominator used only aftersqrtAcalculation succeeds. - Test coverage comprehensive it is, with boundary cases and property checks.
- Clamping and tolerance checks present they are, preventing unstable computations.
- Matrix operations well-conditioned they remain, through careful composition of transformations.
Trust in the Force of this implementation, we can. Unnecessary, additional validation is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases that verify numerical stability
rg -l "numerical|stability|edge.*case|zero|singular" Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp
Length of output: 107
Script:
#!/bin/bash
# Search for test files related to AnnulusBounds
fd "AnnulusBounds.*\.cpp" Tests
# Check for any test cases involving edge cases or boundary conditions
rg -A 5 "TEST|BOOST_AUTO_TEST_CASE" Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp
# Look for any validation of matrix operations
rg "jacobian|matrix|transform" Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp
# Check if there are any small number constants that might indicate numerical threshold checks
rg "epsilon|1[eE]-|0\.[0-9]*" Core/src/Surfaces/AnnulusBounds.cpp
Length of output: 2701
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (1)
73-77: Documentation, this code needs.Clear the logic may be, but mysterious to young padawans it remains. Add comments explaining the negative tolerance handling, you should.
+ // When in Extend mode, only the tolerance check matters if (mode == Extend) { return tolerance.isTolerated(distance, jacobianOpt); + // In Shrink mode, both tolerance and rectangle bounds must be satisfied } else { return tolerance.isTolerated(distance, jacobianOpt) && insideRectangle; }Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (2)
214-242: Strong with the Chi2 tests, the Force is.Both positive and negative Chi2 tolerances, tested thoroughly they are. But hmmmm, identity matrix always used is. Additional tests with different covariance matrices, beneficial they would be.
Add these test cases, you should:
{ auto tolerance = BoundaryTolerance::Chi2Bound(SquareMatrix2::Identity(), 0.1); // ... existing tests ... } +{ + // Test with scaled covariance matrix + SquareMatrix2 scaled = SquareMatrix2::Identity() * 2.0; + auto tolerance = BoundaryTolerance::Chi2Bound(scaled, 0.1); + // ... repeat tests ... +} +{ + // Test with correlated coordinates + SquareMatrix2 correlated; + correlated << 1.0, 0.5, + 0.5, 1.0; + auto tolerance = BoundaryTolerance::Chi2Bound(correlated, 0.1); + // ... repeat tests ... +}Also applies to: 244-272
144-273: Documentation, missing it is.Clear the test cases are, but document their purpose and expected behavior, we should. Comments explaining the tolerance modes and their effects, add you must.
Add documentation like this, you should:
+/** + * Test the behavior of AnnulusBounds with negative tolerances. + * + * Tests four scenarios: + * 1. Positive Euclidean tolerance: Extends the boundary + * 2. Negative Euclidean tolerance: Shrinks the boundary + * 3. Positive Chi2 tolerance: Statistical boundary extension + * 4. Negative Chi2 tolerance: Statistical boundary reduction + */ BOOST_AUTO_TEST_CASE(AnnulusBoundsNegativeTolerance) {Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (3)
30-108: Comprehensive, these constructor tests are! Approve them, I do.Well-structured and thorough, the test coverage is. All constructor variants and tolerance modes, it tests. Both positive and negative cases, it validates. Hmmmm.
A suggestion, I have. Consider adding edge cases, you should:
- Zero covariance matrix for Chi2Bound
- Infinity values for tolerances
- NaN values for tolerances
289-335: Clear and focused, these boundary tests are! Yet improve them further, we can.Good coverage of negative tolerances, I see. Lambda function for reusability, wise choice it is. But magic numbers in test points, clarity they lack.
+ // Constants for test points + const double INSIDE_MARGIN = 0.2; // Distance inside the boundary + const double OUTSIDE_MARGIN = 0.1; // Distance outside the boundary + const double SAFE_INSIDE = 0.3; // Well inside the boundary + { - auto tolerance = BoundaryTolerance::AbsoluteEuclidean(-0.25); + const double NEGATIVE_TOLERANCE = -0.25; + auto tolerance = BoundaryTolerance::AbsoluteEuclidean(NEGATIVE_TOLERANCE);
337-385: Strong with the Force, these trapezoid tests are! Documentation, they need.Comprehensive test coverage, you have achieved. Both positive and negative tolerances, you validate. But clarity in documentation, we seek.
Add these comments, you should:
Vector2 vertices[] = {{1.5, 1}, {2.5, 1}, {3, 3}, {1, 3}}; + // Test points layout: + // (1,3) -------- (3,3) + // | Test / + // | Points / + // | / + // (1.5,1) -- (2.5,1) + + // Test strategy: + // 1. Check points along edges with both positive and negative tolerances + // 2. Verify corners with different tolerance regions + // 3. Test points that should be excluded when boundary shrinksCore/src/Surfaces/BoundaryTolerance.cpp (2)
63-115: Hmm, well structured the new toleranceMode() is, but group similar cases together we should.Sound implementation this is, handling all tolerance types with wisdom. Yet suggest I do, grouping the zero-tolerance checks together for clearer intent, make the code flow more natural it would.
if (isInfinite()) { return Extend; } if (isNone()) { return None; } + + // Handle zero tolerance cases first if (const auto* absoluteBound = getVariantPtr<AbsoluteBound>(); absoluteBound != nullptr) { if (absoluteBound->tolerance0 == 0. && absoluteBound->tolerance1 == 0.) { return None; } return Extend; }
198-202: Handle the zero tolerance edge case explicitly, we should.Clear the implementation is, but explicit handling of zero tolerance, improve readability it would. Prevent future confusion, it will.
if (const auto* absoluteEuclidean = getVariantPtr<AbsoluteEuclidean>(); absoluteEuclidean != nullptr) { + if (absoluteEuclidean->tolerance == 0) { + return cartesianDistance.isZero(); + } if (absoluteEuclidean->tolerance < 0) { return cartesianDistance.norm() > std::abs(absoluteEuclidean->tolerance); } else { return cartesianDistance.norm() <= absoluteEuclidean->tolerance; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(4 hunks)Core/src/Surfaces/BoundaryTolerance.cpp(3 hunks)Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp(2 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)
🔇 Additional comments (9)
Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp (3)
32-47: Hmm, clean and wise these changes are.
A clear path through the code, you have created. Early returns and enum usage, elegant they are. The Force is strong with this implementation.
92-112: Wisdom in comments and optimization, I sense.
Well-structured the code is, with thoughtful TODOs about future optimizations. The path of performance improvement, clearly marked it is.
129-134: Verify the distance negation, we must.
Hmm, mysterious this negation of distance vector is. The TODO comment about sign checking, addressed it must be. Careful verification, this requires.
Run these commands, we shall:
✅ Verification successful
The Force guides me to search deeper into the codebase, hmm. Failed the first attempt did, but give up, we shall not. More comprehensive search patterns, I have crafted, to understand the boundary check implementations and their tests, yes.
Correct, the negation of distance is, young padawan.
Through the Force, see clearly I do. In the polygon boundary check, the negation of distance serves a specific purpose, it does. When in Shrink mode, inward we must look, and thus negate the distance we must. Similar pattern in aligned box implementation, observe I do, where no negation needed it is.
The test cases reveal:
- For Extend mode, positive distance used it is
- For Shrink mode, negative distance required it is, to check inward tolerance
- Consistent with other boundary checks in the codebase, this pattern is
The TODO comment about sign checking, unnecessary it has become. Correct implementation already in place, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of distance negation in boundary checks
rg "distance.*tolerance.*isTolerated" -A 5 -B 5
# Look for test cases involving negative distances
ast-grep --pattern 'TEST$_($_, $_) {
$$$
-distance
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Search for boundary check implementations and tests
rg "isTolerated" -A 5 -B 5
# Look for test files related to boundary checks
fd "Boundary.*Test" --exec cat {}
# Search for distance calculations in boundary checks
rg "distance.*boundary" -A 5 -B 5
# Find related header files
fd "Boundary.*hpp" --exec cat {}
Length of output: 43907
Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (3)
144-152: Well structured, the test setup is, hmmmm.
A clean lambda function for point checking, created you have. Reusability and readability, it brings to the tests.
156-183: Comprehensive testing of positive Euclidean tolerance, I sense.
Good coverage of boundary conditions, you have achieved:
- Radial boundaries: Both inner and outer tested thoroughly
- Axial boundaries: Both positive and negative phi regions explored
- Various distances from boundaries: Clear progression from outside to inside
185-212: Balanced, these negative tolerance tests are.
Mirror the positive tolerance tests, they do, but with stricter boundary conditions. Wisdom in symmetry, there is.
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (1)
Line range hint 1-386: Verify the implementation coverage, we must.
Comprehensive test suite, you have built. But ensure all implementation paths are covered, we should.
✅ Verification successful
Hmm, searched through the codebase, I have. Clear to me now, it is. A final response, I shall generate.
Complete test coverage for BoundaryTolerance, I sense.
Comprehensive test suite for BoundaryTolerance class and its implementations, you have built. All tolerance modes and boundary check scenarios, thoroughly tested they are:
- None tolerance mode
- Infinite tolerance mode
- AbsoluteBound tolerance with positive/negative cases
- AbsoluteCartesian tolerance with positive/negative cases
- AbsoluteEuclidean tolerance with positive/negative cases
- Chi2Bound tolerance with positive/negative cases
For both rectangular (aligned box) and non-rectangular (triangle, trapezoid) boundaries, tested the implementations are. Edge cases and corner cases, covered they also are.
Strong with the Force, this test suite is. No additional verification needed, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BoundaryTolerance class implementation
echo "Searching for BoundaryTolerance implementation..."
ast-grep --pattern 'class BoundaryTolerance {
$$$
}'
# Search for tolerance-related methods
echo "Searching for tolerance-related methods..."
rg -p "tolerance" --type cpp
Length of output: 58647
Core/src/Surfaces/BoundaryTolerance.cpp (2)
173-178: Verify the mathematical correctness of Chi2 calculations, we must.
Introduced support for negative Chi2 bounds, you have. But validate the mathematical foundations, we should. The factor of 2 in both cases, significant it is.
Consider adding documentation explaining the mathematical reasoning behind the factor of 2 and the negative Chi2 interpretation, help future maintainers it will.
✅ Verification successful
Mathematically sound, the Chi2 calculations are, young padawan.
Through the tests and implementation, clear it becomes:
- For positive maxChi2, the factor of 2 accounts for the mean of Mahalanobis distance in 2D space, as documented in the code comments
- For negative maxChi2, the same factor maintains consistency with the positive case, but inverts the boundary check logic
- Test cases validate both positive and negative scenarios with various tolerance values
- The implementation handles both extension (positive maxChi2) and shrinkage (negative maxChi2) of boundaries correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests validating the Chi2 calculations
rg -l "Chi2.*test" --type cpp
Length of output: 88
Script:
#!/bin/bash
# Let's search for the implementation and usage of Chi2Bound
ast-grep --pattern 'struct Chi2Bound {
$$$
}'
# Also search for any documentation or comments about Chi2 calculations
rg -B2 -A2 "Chi2" --type cpp
# Check the test file found earlier
cat Tests/UnitTests/Core/Utilities/AnnealingUtilityTests.cpp
Length of output: 66215
Script:
#!/bin/bash
# Search for tests specifically related to BoundaryTolerance and Chi2Bound
cat Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
Length of output: 14210
Line range hint 63-202: Verify the impact on dependent code, we must.
Changed the behavior of tolerance handling significantly, we have. Ensure all dependent code handles negative tolerances correctly, we must.
✅ Verification successful
Verified, the changes are. Safe, they are.
Extensive test coverage for negative tolerances, I see. Comprehensive test cases exist:
- For AbsoluteEuclidean with negative values, tested they are
- For Chi2Bound with negative values, covered they are
- For boundary checks with negative tolerances, thorough tests exist
Proper handling in implementation and tests, observed I have:
- Shrink mode for negative tolerances, correctly implemented it is
- Test cases for both rectangle and trapezoid boundaries with negative tolerances, present they are
- All dependent code, prepared for negative values it is
The Force is strong with this implementation. Ready for production, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of BoundaryTolerance in test files
echo "Searching for test coverage..."
rg -l "BoundaryTolerance" --type cpp --glob "*test*"
# Search for direct usage in implementation files
echo "Searching for implementation usage..."
rg -l "BoundaryTolerance" --type cpp --glob "!*test*"
Length of output: 6720
Script:
#!/bin/bash
# Search for test cases that verify negative tolerance behavior
echo "Searching for negative tolerance test cases..."
rg -B2 -A2 "negative.*tolerance|tolerance.*negative" Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
# Search for actual usage of isTolerated method
echo "Searching for isTolerated usage..."
rg -B2 -A2 "isTolerated" --type cpp
# Search for test cases involving Chi2Bound with negative values
echo "Searching for Chi2Bound negative test cases..."
rg -B2 -A2 "Chi2Bound.*negative|negative.*Chi2Bound" Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
# Search for AbsoluteEuclidean negative test cases
echo "Searching for AbsoluteEuclidean negative test cases..."
rg -B2 -A2 "AbsoluteEuclidean.*negative|negative.*AbsoluteEuclidean" Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
Length of output: 2972
Script:
#!/bin/bash
# Search for test cases in BoundaryToleranceTests.cpp
echo "Searching full BoundaryToleranceTests content..."
cat Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
# Search for usage of toleranceMode() to verify handling of negative cases
echo "Searching for toleranceMode usage..."
rg -B3 -A3 "toleranceMode\(\)" --type cpp
# Check implementation files that use BoundaryTolerance for negative value handling
echo "Checking Surface implementations..."
rg -B2 -A2 "BoundaryTolerance::(AbsoluteEuclidean|Chi2Bound)" Core/src/Surfaces/*.cpp
Length of output: 24729
|
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Extend,None, andShrink.Bug Fixes
Tests
AnnulusBoundsandBoundaryToleranceto validate behavior with negative tolerances and constructor inputs.