Fix #179 #178
Open
Fix #179 #178
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.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #178 +/- ##
==========================================
- Coverage 54.88% 54.31% -0.57%
==========================================
Files 24 27 +3
Lines 2551 2629 +78
==========================================
+ Hits 1400 1428 +28
- Misses 1151 1201 +50 ☔ View full report in Codecov by Sentry. |
janbridley
force-pushed
the
fix-minimal_bounding_sphere
branch
from
2863227 to
f0b8598
Compare
Replacing the ``miniball`` package with the improved ``cyminiball`` c++ binding allows for accurate calculation of a minimal_bounding_sphere for complex polyhedra. An internal test to ensure vertices are within (or extremely close to) the minimal bounding sphere confirms expected behavior and catches cases where an incorrect miniball could be calculated.
bdice
requested changes
Feb 21, 2023
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| break | ||
| except np.linalg.LinAlgError: | ||
| current_rotation = rowan.random.rand(1) | ||
| vertices = rowan.rotate(current_rotation, vertices) | ||
| except AssertionError: |
There was a problem hiding this comment.
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): |
|
@janbridley FYI, I merged in the upstream to fix CI failures. |
Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
8 tasks
Tobias-Dwyer
pushed a commit
that referenced
this pull request
Mar 27, 2023
… solids. (#177) * 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. * Added json files for new shape families as well as accompanying import functions. * Added new pytest marks and a few test functions. * 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. * Tightnened tolerance for test_insphere and increased deadline Tightened ``atol`` of ``test_insphere``. Increased the deadline of the test to accommodate testing of shapes with many faces (e.g. disdyakis triacontahedron). * Applied test_bounding_sphere to archimedean solids * Tightened tolerance for test_bounding_sphere * Added marks for prism/antiprism and pyramid/dipyramid families * Applied test_get_set_minimal_centered_bounding_sphere_radius to all new shape families, including prism/antiprism and pyramid/dipyramid * Applied test_volume and test_surface_area to prism/antiprism and pyramid/dipyramid. Fixed typo in conftest * Formatted code with black. * Revert commit "Added new classes to return the edges of polyhedra" This commit reverts changes made in ab21279, which are included in a different PR. * Fixed docstring for common.py * Split test_insphere into multiple functions based on input data complexity. 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. * Increased resilience of minimal_bounding_sphere method 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. * Revert "Increased resilience of minimal_bounding_sphere method" This reverts commit 40f500c, moving all miniball-related changes to #178. * Removed unnecessary print statement in the combine_mark function * Updated Credits.rst to not conflict with master * Fixed missing space in archimedean.json "Truncated Cuboctahedron" * Added additional pytest for json data and new families --------- Co-authored-by: Brandon Butler <[email protected]> Co-authored-by: Vyas Ramasubramani <[email protected]>
8 tasks
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
minimal_bounding_spherewill fail for a number of augmented Johnson solids, due to numerical imprecision inherent to theminiballpackage. Switching to the faster, more robustcyminiballsolver (see #179, here) will improve the calculated results and implementing an internal check will ensure calculated results are accurate to an arbitrarily high degree of precision - in this case, 1e-15 or approximately 10*machine epsilon.Possible discussion
Current worst case runtime observed over 15000 tests on an m1 Mac is 0.3 seconds, with an average runtime of 0.006s and a 1% high of 0.1s. Reducing precision can accelerate the worst case, but does not significantly change the 1% highs. Note that in every case, the miniball for augmented Johnson solids takes far longer than simple polyhedra so most users (those who primarily test Platonic, Archimedean, and Catalan solids) will not be negatively impacted by this change. This behavior is expected, as instability in the miniball solver increases with increasing degree of asphericity.
Types of Changes
Checklist: