-
Notifications
You must be signed in to change notification settings - Fork 5
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
Performance-focused rewrite of the ConvexPolyhedron class #188
Conversation
Only test quaternions that are not effectively zero.
The new class ``polyhedron.edges`` returns the a list of the edges of a polyhedron as vertex-index pairs, similar to the current ``polyhedron.faces`` class. The class ``polyhedron.get_edge_vectors`` returns a list of edges as vectors in 3D space.
Co-authored-by: Vyas Ramasubramani <[email protected]>
Co-authored-by: Vyas Ramasubramani <[email protected]>
Until the precision improvements in #177 are added, a decrease in rtol and the merge_faces call are required for pytest to succeed on the dodecahedron's edges. Once the precision is increased, these temporary solutions can be reverted
…ed data Currently, the stored data for polyhedra is not accurate enough for the ``edges`` and ``edge_vectors`` properties to function as expected. Once #177 is merged, the accuracy increase should fix the issue and assertion tolerances for testing can be tightened. In addition, ``merge_faces`` should no longer be required for any of the polyhedra included with this package.
Co-authored-by: Bradley Dice <[email protected]>
…oxeter into release-edgelengths
After discussing possible options for returning polyhedral edges, it was determined that a set of i<j pairs of vertices was the best option for output in the ``edges`` method. A description of how to generate (j,i) pairs was added to the docstring, and the edge_vectors method was updated to work with the new set/tuple structure.
It was determined that edges should be returned as an ordered Numpy array. This commit ensures the final output is a numpy array, sorted first by the (i) element and then by the (j) element where i<j. This mimics Mathematica's edges output format and should aid in usability. Documentation was updated accordingly.
for more information, see https://pre-commit.ci
…and functools caching
Co-authored-by: Domagoj Fijan <[email protected]>
|
||
# Get the absolute angles of each vertex and fit to unit circle | ||
angles = np.arctan2(vertices[:, 1], vertices[:, 0]) | ||
angles = np.mod(angles - angles[0], 2 * np.pi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? This just changes which vertex you start at basically
right? I might be missing something. arctan2 codomain is -pi to pi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pytests for ConvexPolyhedron pass with or without the shift, but the current code mirrors the behavior of shapes/_reorder_verts
. Removing the rescaling from ConvexPolygon causes the test test_polygon.p::test_nonregular_convex_polygon_distance_to_surface_unit_area_ngon
to fail. It seems that reordering the vertices is somehow required to ensure the resulting polygon is simple. On a first glance I can't figure out why this would be the case, but I think we should explore further because the behavior is quite odd. This also highlights that we should add some 3D test cases with non regular faces - the current named_solids_mark
does not include anything more irregular than Johnson solids
While the above comment is mostly correct, the damasceno_shapes_mark
does test seven irregular polyhedra (O15-O22). The EllipsoidSurfaceStrategy
also tests irregular shapes, although almost all of them will be deltahedra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments that we should discuss. Some methods are lacking tests that should be added. And some minor tweaks are suggested.
…ab/coxeter into update-ConvexPolyhedron
…ab/coxeter into update-ConvexPolyhedron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any more suggestions for improvement.This looks fine and since tests pass I think this might be good to go.
@tcmoore3 Any other feedback? If not I will merge in the morning and start working on a release |
Prior to this PR, the ConvexPolyhedron class used Scipy's (Qhull) convex hull to compute faces of the solid. The shape was then initialized in the same manner as a standard Polyhedron. This rewrite stores simplices and other data from the convex hull, which can be used to accelerate many of the classes methods. Slow and commonly run functions have been rewritten for performance, and new and/or improved pytests have been written.
Description
The new class builds a convex hull, then stores most of the data from that object. This data, especially the list of simplices, allow for numpy vectorization of many functions that required a for loop in the previous code. These simplices can be sorted to ensure math involving the curl/divergence theorems works properly, and can be merged based on common normals to build faces similar to the current merge_faces function.
The following methods have been reimplemented, and a brief description is given:
find_equations
: vectorized using numpy_surface_triangulation
: updated to use precalculated simplices, so polytri is not requiredvolume
,surface_area
: reimplemented to use stored volume data for the calculations.centroid
: rewritten to calculate from a list of simplices. Vectorized using numpy, and computes solutions using (_centroid_from_triangulated_surface
)_compute_inertia_tensor
: fully rewritten for performance, using an algorithm that works with tetrahedral decompositions that include negative-volume tetrahedra. It is also significantly faster._rescale
: updated to properly scale all stored properties. Also recalculates the centroid to ensure proper behavior.get_face_area
: vectorized with numpyThe following new methods have been added:
_combine simplices
: faster alternative to merge faces, but only works on simplices_find_simplex_equations
: similar to _find_equations, but works on every simplex rather than every face_find_coplanar_simplices
: helper function that finds simplices with the same plane equation (within a tolerance)._calculate_signed_volume
: faster method of computing volume via a tetrahedral decomposition. Used as an internal check that volume is stable when modifying the shape_find_face_centroids
: used with the new face_centroids property. Quickly finds the center of mass for each of the polygonal facesThe following properties have been added:
simplices
equations
face_centroids
Motivation and Context
Creation of a ConvexPolyhedron object takes on the order of 1e-2s - this is fine when generating small numbers of shapes, but can become a significant hindrance when generating tens or hundreds of thousands of shapes. In addition, many commonly used methods (especially volume, centroid, and inertia tensor) took on the order of 1e-3s and did not cache their data. These changes address these performance issues, and add a wide range of private variables which can be used to more quickly calculate geometric properties that have not yet been implemented. Benchmarks are included below, but in general this rewrite provides a 10x-100x performance improvement on improved methods. For properties with caching, values are only recalculated when the shape is modified and access the data takes on the order of 1e-8s
Benchmarks
The chart above shows the runtimes of common actions averages over 10 thousand samples of each Johnson solid. Errorbars show the standard deviation between shapes.
Note that tests were performed in much more depth than shown below - these are simply a subset of the total
With optimization
Cube (V=8, F=6, E=12)
Metabidiminished rhombicosidodecahedron (V=50, F=42, E=90)
Without optimization (previous versions of coxeter)
Cube (V=8, F=6, E=12)
Metabidiminished rhombicosidodecahedron (V=50, F=42, E=90)
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist: