Skip to content

Added new classes to return the edges of polyhedra #171

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

Merged
merged 35 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
f0b8598
test: Fix assumption on quaternions.
b-butler Jan 25, 2023
ab21279
Added new classes to return the edges of polyhedra
janbridley Feb 6, 2023
22399d8
Fixed return for get_edge_vectors method.
janbridley Feb 7, 2023
c9108cd
Merge branch 'master' into release-edgelengths
janbridley Feb 13, 2023
7c94100
Renamed get_edge_vectors property to edge_vectors
janbridley Feb 24, 2023
0e6501a
Vectorized edge_vectors return with numpy
janbridley Feb 27, 2023
fdec57c
Updated test_polyhedron to be compatible with the renamed edge_vector…
janbridley Feb 27, 2023
dd47ed2
Updated test_polyhedron to explicitly cover the edges property
janbridley Feb 27, 2023
210f009
Updated rtol for edge property test
janbridley Feb 27, 2023
a2cfd3c
Reverted small fixes that account for inaccuracies in polyhedron stor…
janbridley Feb 27, 2023
1f4a106
Removed unnecessary comment from ``test_edges``
janbridley Feb 28, 2023
d387fca
Changed ``edge_vectors`` property to return a numpy array
janbridley Feb 28, 2023
6e69875
Merge branch 'release-edgelengths' of https://github.com/glotzerlab/c…
janbridley Feb 28, 2023
f473e27
Rewrote ``edges`` method and updated ``edge_vectors`` for compatibility
janbridley Mar 22, 2023
cd67d45
Updated ``test_polyhedron`` to work with set edges
janbridley Mar 22, 2023
01077cc
Fixed docstring formatting for ``edges`` and ``edge_vectors``
janbridley Mar 22, 2023
ada7029
Updated edges method to return Numpy array
janbridley May 25, 2023
7a89acb
test_polyhedron::test_edges now checks edge count
janbridley May 25, 2023
b03099b
Merge branch 'master' into release-edgelengths
janbridley May 25, 2023
770199b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 25, 2023
a879cc2
Merge branch 'master' into release-edgelengths
janbridley Jun 26, 2023
b4f60f9
Updated edges property to cache result
janbridley Aug 4, 2023
6b72414
Updated edges and edge_vectors to make better use of numpy functions …
janbridley Aug 7, 2023
ce495d9
Added num_edges method
janbridley Aug 7, 2023
ced2fe6
Updated credits
janbridley Aug 7, 2023
20a8518
Updated test_polyhedron to test num_edges property
janbridley Aug 7, 2023
3a2f336
Updated edges documentation
janbridley Aug 8, 2023
f730e0c
Updated edge_vectors documentation
janbridley Aug 8, 2023
986fa8d
Refactored test_edges to be more comprehensive
janbridley Aug 8, 2023
3c87b94
Merge branch 'master' into release-edgelengths
janbridley Aug 8, 2023
d3f5203
Added fast num_edges calculation for convex polyhedron and improved p…
janbridley Aug 8, 2023
2a325e6
test_num_edges now covers nonconvex Polyhedron class
janbridley Aug 9, 2023
ec798c7
Update Credits.rst
janbridley Aug 11, 2023
8d4b361
Test i-i edges
janbridley Aug 14, 2023
341cd53
Removed changes to .gitignore
janbridley Aug 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Credits.rst
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ Jen Bradley
* Added shape families for Archimedean, Catalan, and Johnson solids.
* Added shape family for prisms and antiprisms.
* Added shape family for equilateral pyramids and dipyramids.
* Added edges, edge_vectors, and num_edges methods.

Domagoj Fijan
* Rewrote point in polygon check to use NumPy vectorized operations.
Expand Down
6 changes: 6 additions & 0 deletions coxeter/shapes/convex_polyhedron.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ def asphericity(self):
"""float: Get the asphericity as defined in :cite:`Irrgang2017`."""
return self.mean_curvature * self.surface_area / (3 * self.volume)

@property
def num_edges(self):
"""int: Get the number of edges."""
# Calculate number of edges from Euler Characteristic
return self.num_vertices + self.num_faces - 2

def is_inside(self, points):
"""Determine whether points are contained in this polyhedron.

Expand Down
39 changes: 38 additions & 1 deletion coxeter/shapes/polyhedron.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"""Defines a polyhedron."""

import warnings
from functools import cached_property

import numpy as np
import rowan
Expand Down Expand Up @@ -356,9 +357,45 @@ def vertices(self):

@property
def faces(self):
"""list(:class:`numpy.ndarray`): Get the polyhedron's faces."""
"""list(:class:`numpy.ndarray`): Get the polyhedron's faces.

Results returned as vertex index lists.
"""
return self._faces

@cached_property
def edges(self):
""":class:`numpy.ndarray`: Get the polyhedron's edges.

Results returned as vertex index pairs, with each edge of the polyhedron
included exactly once. Edge (i,j) pairs are ordered by vertex index with i<j.
"""
ij_pairs = np.array(
[
[i, j]
for face in self.faces
for i, j in zip(face, np.roll(face, -1))
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure — the face indices are ordered consistently, right? In other words, this code relies on face indices being sorted in such a way that any shared edge is in the order (i, j) for one face and (j, i) for the other face sharing that edge. Is that a safe assumption from the rest of the code (it has been a while since I’ve read it). If this isn’t handled properly by the construction of the shape, then you’ll get missing edges. Are there any degenerate cases where this doesn’t happen automatically (can a polyhedron have one face in coxeter)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am unsure exactly what the limits of Polyhedron are: for example, if self-intersecting shapes, shapes with holes, etc work properly. That being said, faces are ordered and edges are built using another method during initialization so it must work for the vast majority of use cases! I can take a deeper look in the next few days.

Copy link
Collaborator Author

@janbridley janbridley Aug 14, 2023

Choose a reason for hiding this comment

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

Complex polyhedra (within limits) seem to work - self intersecting polyhedra, polyhedra with holes, and simple non convex (e.g. stellated) polyhedra all compute edges as expected. However, there are cases where volumes, moments of inertia, and other properties are incorrect. There are some cases where edges does NOT work - but in these cases, many other methods break down. Polygons (one-face polyhedron), dihedra, and nonclosed solids tend to break this method but they also return null values for any method that includes volume. Some also have erroneous behavior in the centroid or inertia tensor calculation. I will create an issue for some of these edge cases in general, but overall the new edge methods are at least as robust as the other core methods.

Copy link
Member

@bdice bdice Aug 14, 2023

Choose a reason for hiding this comment

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

Good. It’s good to know the limitations — and which cases you are willing to treat as undefined behavior vs. raising an error or “fix” in some way.

if i < j
]
)
sorted_indices = np.lexsort(ij_pairs.T[::-1])
sorted_ij_pairs = ij_pairs[sorted_indices]
# Make edge data read-only so that the cached property of this instance
# cannot be edited
sorted_ij_pairs.flags.writeable = False

return sorted_ij_pairs

@property
def edge_vectors(self):
""":class:`numpy.ndarray`: Get the polyhedron's edges as vectors."""
return self.vertices[self.edges[:, 1]] - self.vertices[self.edges[:, 0]]

@property
def num_edges(self):
"""int: Get the number of edges."""
return len(self.edges)

@property
def volume(self):
"""float: Get or set the polyhedron's volume."""
Expand Down
92 changes: 91 additions & 1 deletion tests/test_polyhedron.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
named_pyramiddipyramid_mark,
sphere_isclose,
)
from coxeter.families import DOI_SHAPE_REPOSITORIES, PlatonicFamily
from coxeter.families import DOI_SHAPE_REPOSITORIES, ArchimedeanFamily, PlatonicFamily
from coxeter.shapes import ConvexPolyhedron, Polyhedron
from coxeter.shapes.utils import rotate_order2_tensor, translate_inertia_tensor
from utils import compute_centroid_mc, compute_inertia_mc
Expand Down Expand Up @@ -339,6 +339,96 @@ def test___repr__():
repr(icosidodecahedron)


@combine_marks(
named_platonic_mark,
named_archimedean_mark,
named_catalan_mark,
named_johnson_mark,
named_prismantiprism_mark,
named_pyramiddipyramid_mark,
)
def test_edges(poly):
# Check that the first column is in ascending order.
assert np.all(np.diff(poly.edges[:, 0]) >= 0)

# Check that all items in the first column are greater than those in the second.
assert np.all(np.diff(poly.edges, axis=1) > 0)

# Check the second column is in ascending order for each unique item in the first.
# For example, [[0,1],[0,3],[1,2]] is permitted but [[0,1],[0,3],[0,2]] is not.
edges = poly.edges
unique_values = unique_values = np.unique(edges[:, 0])
assert all(
[
np.all(np.diff(edges[edges[:, 0] == value, 1]) >= 0)
for value in unique_values
]
)

# Check that there are no duplicate edges. This also double-checks the sorting
assert np.all(np.unique(poly.edges, axis=1) == poly.edges)

# Check that the edges are immutable
Copy link
Member

Choose a reason for hiding this comment

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

This can be simpler if you want. I would just use pytest’s “assert raises” context manager, wrapping the assignment.

try:
poly.edges[1] = [99, 99]
# If the assignment works, catch that:
assert poly.edges[1] != [99, 99]
except ValueError as ve:
assert "read-only" in str(ve)


def test_edge_lengths():
known_shapes = {
"Tetrahedron": np.sqrt(2) * np.cbrt(3),
"Cube": 1,
"Octahedron": np.power(2, 5 / 6) * np.cbrt(3 / 8),
"Dodecahedron": np.power(2, 2 / 3) * np.cbrt(1 / (15 + np.sqrt(245))),
"Icosahedron": np.cbrt(9 / 5 - 3 / 5 * np.sqrt(5)),
}
for name, edgelength in known_shapes.items():
poly = PlatonicFamily.get_shape(name)
# Check that edge lengths are correct
veclens = np.linalg.norm(
poly.vertices[poly.edges[:, 1]] - poly.vertices[poly.edges[:, 0]], axis=1
)
assert np.allclose(veclens, edgelength)
assert np.allclose(veclens, np.linalg.norm(poly.edge_vectors, axis=1))


def test_num_edges_archimedean():
known_shapes = {
"Cuboctahedron": 24,
"Icosidodecahedron": 60,
"Truncated Tetrahedron": 18,
"Truncated Octahedron": 36,
"Truncated Cube": 36,
"Truncated Icosahedron": 90,
"Truncated Dodecahedron": 90,
"Rhombicuboctahedron": 48,
"Rhombicosidodecahedron": 120,
"Truncated Cuboctahedron": 72,
"Truncated Icosidodecahedron": 180,
"Snub Cuboctahedron": 60,
"Snub Icosidodecahedron": 150,
}
for name, num_edges in known_shapes.items():
poly = ArchimedeanFamily.get_shape(name)
assert poly.num_edges == num_edges


@given(
EllipsoidSurfaceStrategy,
)
def test_num_edges_polyhedron(points):
hull = ConvexHull(points)
poly = ConvexPolyhedron(points[hull.vertices])
ppoly = Polyhedron(poly.vertices, poly.faces)

# Calculate correct number of edges from euler characteristic
euler_characteristic_edge_count = ppoly.num_vertices + ppoly.num_faces - 2
assert ppoly.num_edges == euler_characteristic_edge_count


def test_curvature():
"""Regression test against values computed with older method."""
# The shapes in the PlatonicFamily are normalized to unit volume.
Expand Down