Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #179 #178

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
10 changes: 5 additions & 5 deletions coxeter/shapes/polygon.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from .utils import _generate_ax, rotate_order2_tensor, translate_inertia_tensor

try:
import miniball
import cyminiball

MINIBALL = True
except ImportError:
Expand Down Expand Up @@ -475,13 +475,13 @@ def minimal_bounding_circle(self):
""":class:`~.Circle`: Get the minimal bounding circle."""
if not MINIBALL:
raise ImportError(
"The miniball module must be installed. It can "
"The cyminiball module must be installed. It can "
"be installed as an extra with coxeter (e.g. "
"with pip install coxeter[bounding_sphere], or "
"directly from PyPI using pip install miniball."
"directly from PyPI using pip install cyminiball."
)

# The algorithm in miniball involves solving a linear system and
# The algorithm in cyminiball involves solving a linear system and
# can therefore occasionally be somewhat unstable. Applying a
# random rotation will usually fix the issue.
max_attempts = 10
Expand All @@ -491,7 +491,7 @@ def minimal_bounding_circle(self):
while attempt < max_attempts:
attempt += 1
try:
center, r2 = miniball.get_bounding_ball(vertices)
center, r2 = cyminiball.compute(vertices)
break
except np.linalg.LinAlgError:
current_rotation = rowan.random.rand(1)
Expand Down
31 changes: 22 additions & 9 deletions coxeter/shapes/polyhedron.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from .utils import _generate_ax, _set_3d_axes_equal, translate_inertia_tensor

try:
import miniball
import cyminiball

MINIBALL = True
except ImportError:
Expand Down Expand Up @@ -550,27 +550,40 @@ def minimal_bounding_sphere(self):
""":class:`~.Sphere`: Get the polyhedron's bounding sphere."""
if not MINIBALL:
raise ImportError(
"The miniball module must be installed. It can "
"The cyminiball module must be installed. It can "
"be installed as an extra with coxeter (e.g. "
'with "pip install coxeter[bounding_sphere]") or '
'directly from PyPI using "pip install miniball".'
'directly from PyPI using "pip install cyminiball".'
)

# The algorithm in miniball involves solving a linear system and
# can therefore occasionally be somewhat unstable. Applying a
# random rotation will usually fix the issue.
max_attempts = 10
# The miniball algorithm (in this case, Gärtner's miniball) can
# experience numerical instability of ~10x machine epsilon. If an incorrect
# miniball is found, applying random rotations will usually fix the issue.

# The following subfunction checks if all vertices lie within the
# minimal bounding sphere, to a tolerance of 1e-15.
def verify_inside(points, center, radius, eps=1e-15):
return np.all(np.linalg.norm(points - center, axis=1) <= radius + eps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we compare squared distances? If we avoid the square root in the norm here and just use sum-of-squares, we can also reuse the value of r2 below without incurring another sqrt. Just depends on whether you think the epsilon is more meaningful in terms of distance rather than distance squared, I suppose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For small distances (e.g. for points that are very close to the bounding sphere), ε will be more significant and therefore represent a larger tolerance range for accepted points. This is magnified for square distances, so if we do make this change we should probably test a smaller ε. I will set this aside while I figure out hbf/miniball and we can revisit later


# Simple polyhedra (e.g. dodecahedra) will require far less than 200 attempts
# to compute a correct miniball. Polyhedra with augmentations and large numbers
# of vertices will take more, but none are likely to exceed 200 attempts. Worst-
# case runtime is approximately 0.25s, and occurrs in ~5/1e5 samples.
janbridley marked this conversation as resolved.
Show resolved Hide resolved
max_attempts = 200
attempt = 0
current_rotation = [1, 0, 0, 0]
vertices = self.vertices
while attempt < max_attempts:
attempt += 1
try:
center, r2 = miniball.get_bounding_ball(vertices)
center, r2 = cyminiball.compute(vertices)
assert verify_inside(vertices, center, np.sqrt(r2))
break
except np.linalg.LinAlgError:
current_rotation = rowan.random.rand(1)
vertices = rowan.rotate(current_rotation, vertices)
except AssertionError:
Copy link
Member

@bdice bdice Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s combine this with the previous LinAlgError and handle both at the same time, since the subsequent code paths are identical. Should look something like:

Suggested change
except AssertionError:
except (np.linalg.LinAlgError, AssertionError):

current_rotation = rowan.random.rand(1)
vertices = rowan.rotate(current_rotation, vertices)
janbridley marked this conversation as resolved.
Show resolved Hide resolved
else:
raise RuntimeError("Unable to solve for a bounding sphere.")

Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
]

bounding_deps = [
"miniball",
"cyminiball",
]

extras = {
Expand Down