fix: resolve numerical failures and premature caching in ComputeInnerBall#468
Open
devangpratap wants to merge 2 commits intoGeomScale:developfrom
Open
fix: resolve numerical failures and premature caching in ComputeInnerBall#468devangpratap wants to merge 2 commits intoGeomScale:developfrom
devangpratap wants to merge 2 commits intoGeomScale:developfrom
Conversation
added 2 commits
March 3, 2026 21:30
GeomScale#463) Three bugs caused garbage data to propagate when both max_inscribed_ball and lpsolve fail on non-full-dimensional H-polytopes: 1. has_ball was set true before computation completed, so a subsequent lpsolve failure (exception_pair with Point(1) and radius -1) would be permanently cached as a valid result with a mismatched dimension. 2. ComputeChebychevBall's return value was stored without validation. For an exactly degenerate polytope lpsolve returns OPTIMAL with r=0, which callers checking (radius < 0.0) would not catch, leading to division by zero downstream. For numerical failures it returns exception_pair with radius -1 and a 1-dimensional center regardless of polytope dimension. 3. The DISABLE_LPSOLVE failure path reset has_ball to false but returned _inner_ball whose second member (NT/double) was never written, leaving the radius indeterminate. Fix: validate the lpsolve result against tol/2.0 (mirroring the existing check on max_inscribed_ball's output). On failure return {Point(_d), -1} directly without caching, giving callers the proper-dimension zero center and the -1 sentinel they already expect. has_ball is only set on success. Add a regression test: a 2D square embedded in 3D space via x3=0 as a pair of inequality constraints. Both solvers correctly find no positive- radius ball; the fix ensures the returned pair has dimension 3 (not 1) and radius -1 (not 0 or indeterminate), and that repeated calls do not cache a false success.
Ran clang-format -style="{BasedOnStyle: LLVM, IndentWidth: 4}" on both
files to bring them in line with project style requirements. No logic
changes; only whitespace, brace placement, and line-wrapping adjustments.
vissarion
requested changes
Mar 12, 2026
Member
vissarion
left a comment
vissarion
left a comment
There was a problem hiding this comment.
Thanks for this PR. Could you please remove all changes related to style and reordering of commands to simplify review and focus on the real changes of the PR.
Author
Understood, working on it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
##Overview
This PR addresses three critical stability issues in ComputeInnerBall within the volesti core. These bugs previously allowed for garbage data propagation and segmentation faults when handling non-full-dimensional H-polytopes.
Root Causes Identified
Technical Fixes
Regression Testing
Added a new test case in test/test_internal_points.cpp: