Added new shape families for Archimedean, Catalan, Johnson, and other solids. #177
Conversation
This file contains 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
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.
* New pytest marks have been created for the Archimedean, Catalan, and Johnson shape families. * A function has been written to combine multiple marks into a single mark for more compact testing of shape properties. * test_volume function for damasceno shapes has been adapted to test the new shape families * A test_surface_area method similar to test_volume has been added.
Tightened ``atol`` of ``test_insphere``. Increased the deadline of the test to accommodate testing of shapes with many faces (e.g. disdyakis triacontahedron).
…ew shape families, including prism/antiprism and pyramid/dipyramid
…mid/dipyramid. Fixed typo in conftest
This commit reverts changes made in ab21279, which are included in a different PR.
…exity. The Random3DRotationStrategy test varies widely in runtime based on the complexity of the input shape. Splitting the calling function into two tests (for simple Platonic shapes and more complex Catalan shapes) should solve this issue.
…thub.com/glotzerlab/coxeter into release-recalculate_renormalize_platonic
janbridley
force-pushed
the
release-recalculate_renormalize_platonic
branch
from
52533ec to
2df65c0
Compare
The ``minimal_bounding_sphere`` method is reliant on the solution to a linear system of equations. For polyhedra with portions that protrude out from the main mass, this method can fail due to numerical instability in a small percent of cases. This fix double-checks that the calculated miniball contains all points on the polyhedron, and allows the method to make more attempts to find a correct answer before failing.
vyasr
requested changes
Feb 23, 2023
There was a problem hiding this comment.
Some minor suggestions here, as well as a request to clean out miniball-related changes. Aside from that, is there anyone else in the group who could confirm the validity of the shape families themselves? I am not working with these currently and it would be good for someone who is to take a few minutes to spot check that the produced shapes are valid.
janbridley
added a commit
that referenced
this pull request
Feb 27, 2023
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
7 tasks
janbridley
added a commit
that referenced
this pull request
Feb 27, 2023
…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.
Codecov Report
@@ Coverage Diff @@
## master #177 +/- ##
==========================================
+ Coverage 54.44% 54.45% +0.01%
==========================================
Files 27 27
Lines 2623 2628 +5
==========================================
+ Hits 1428 1431 +3
- Misses 1195 1197 +2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
vyasr
approved these changes
Mar 27, 2023
|
|
||
| # A convenient mark decorator that also includes names for the polyhedra. | ||
| # Assumes that the argument name is "poly". | ||
| _archimedean_shape_names = ArchimedeanFamily.data.keys() |
There was a problem hiding this comment.
FYI you don't need to extract keys to iterate over a dictionary, and it's usually more performant not to do so:
In [3]: for key in dict.fromkeys('abcdefg'):print(key)
a
b
c
d
e
f
g
In [4]: %timeit for key in dict.fromkeys('abcdefg'): pass
629 ns _ 8.29 ns per loop (mean _ std. dev. of 7 runs, 1,000,000 loops each)
In [5]: %timeit for key in dict.fromkeys('abcdefg').keys(): pass
711 ns _ 6.37 ns per loop (mean _ std. dev. of 7 runs, 1,000,000 loops each)
There was a problem hiding this comment.
Thanks for the feedback, and noted for next time!
|
@janbridley feel free to click the "Squash and merge" button when you're ready! |
janbridley
added a commit
that referenced
this pull request
Aug 16, 2023
* test: Fix assumption on quaternions. Only test quaternions that are not effectively zero. * Added new classes to return the edges of polyhedra 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. * Fixed return for get_edge_vectors method. * Renamed get_edge_vectors property to edge_vectors Co-authored-by: Vyas Ramasubramani <[email protected]> * Vectorized edge_vectors return with numpy Co-authored-by: Vyas Ramasubramani <[email protected]> * Updated test_polyhedron to be compatible with the renamed edge_vectors property * Updated test_polyhedron to explicitly cover the edges property * Updated rtol for edge property test 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 * Reverted small fixes that account for inaccuracies in polyhedron stored 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. * Removed unnecessary comment from ``test_edges`` * Changed ``edge_vectors`` property to return a numpy array Co-authored-by: Bradley Dice <[email protected]> * Rewrote ``edges`` method and updated ``edge_vectors`` for compatibility 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. * Updated ``test_polyhedron`` to work with set edges * Fixed docstring formatting for ``edges`` and ``edge_vectors`` * Updated edges method to return Numpy array 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. * test_polyhedron::test_edges now checks edge count * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Updated edges property to cache result * Updated edges and edge_vectors to make better use of numpy functions and functools caching * Added num_edges method * Updated credits * Updated test_polyhedron to test num_edges property * Updated edges documentation Co-authored-by: Bradley Dice <[email protected]> * Updated edge_vectors documentation Co-authored-by: Bradley Dice <[email protected]> * Refactored test_edges to be more comprehensive Added explicit tests for sorting, double checks for edge length, and an additional test for the number of edges based on the euler characteristic * Added fast num_edges calculation for convex polyhedron and improved pytests * test_num_edges now covers nonconvex Polyhedron class * Update Credits.rst Co-authored-by: Bradley Dice <[email protected]> * Test i-i edges Co-authored-by: Bradley Dice <[email protected]> * Removed changes to .gitignore --------- Co-authored-by: Brandon Butler <[email protected]> Co-authored-by: Vyas Ramasubramani <[email protected]> Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
janbridley
added a commit
that referenced
this pull request
Aug 29, 2023
This was referenced Aug 29, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Expanded on coxeter.families with methods and data for several families of common convex polyhedra:
ArchimedeanFamily: The 13 Archimedean solids.CatalanFamily: The 13 Archimedean duals.JohnsonFamily: The 92 Johnson solids.PrismAntiprismFamily: The 16 n-gonal prisms and antiprisms ranging from n=3 to n=10.PyramidDipyramidFamily: The 6 equilateral pyramids and dipyramids.Each new family is queryable with
get_shape("Shape Name")and returns aConvexPolyhedronnormalized to volume of 1, just asPlatonicFamilycurrently does. Vertex data for all new families is stored as a float64, and stored data for the currentPlatonicFamilyhas been updated to match that precision. This change fixes intermittent bugs that arose from lack of numerical precision. New pytests have been added to verify the new families behave as expected.Motivation and Context
Querying polyhedra other than Platonic solids could be relatively difficult. This release streamlines the process and allows for simple access to a greatly increased number of convex solids.
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist: