Optimise RotationAnalyser permutation loop#12
Merged
Conversation
… approach Use bsym to reduce vertex permutations to symmetry-inequivalent representatives, then compose the best-fit Procrustes rotation with the point group's proper rotation matrices to recover all orientation candidates. A single PointGroupAnalyzer call provides both the reduced permutations and the 3x3 rotation matrices. Gives ~50x speedup for octahedral geometries (15 CSM evaluations instead of 720). The composition is exact for undistorted geometries and numerically indistinguishable from brute force for moderate distortions. The docstring documents an orbit-expansion alternative for cases requiring exact rotation matrices. Also fixes the OrientationDict type annotation for all_rotational_distances (np.ndarray, not float).
There was a problem hiding this comment.
Pull request overview
This PR optimizes RotationAnalyser.discrete_orientation by replacing the factorial permutation search with a symmetry-reduced, two-phase approach that uses point-group analysis to drastically reduce CSM evaluations while still enumerating orientation candidates.
Changes:
- Add a lazy, cached point-group analysis step that yields symmetry-reduced permutations and proper rotation matrices.
- Rework
discrete_orientationto use reduced permutations for best alignment, then compose the best-fit rotation with proper symmetry rotations to generate orientation candidates. - Add functional tests for
discrete_orientationand fixOrientationDict.all_rotational_distancestyping tonp.ndarray.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
polyhedral_analysis/rotation_analyser.py |
Implements symmetry-analysis caching and the new two-phase discrete orientation algorithm; updates type annotation. |
tests/test_rotation_analyser.py |
Adds functional tests covering correct reference detection, inverted reference, and perfect-geometry behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove repeated feature description from intro paragraph and correct "stereographic projection plotting" to "orientation distribution plotting" (the plotting module uses a KDE contour plot, not a stereographic projection).
- Track per-reference CSM so returned symmetry_measure matches the chosen reference geometry, not the global best. - Fix docstring array shapes: Mx3xN -> MxNx3, 3xN -> Nx3, etc. - Document that all reference orientations must share the same point group (symmetry analysis is performed once from the first). - Simplify Phase 2 loop by storing per-reference best rotations in Phase 1.
- Validate coord_list_mapping produces a bijection - Guard against empty proper_rotations array - Use det > 0.5 tolerance for proper rotation filtering - Fix list aliasing in best_rotation_per_ref initialisation - Bundle lazy cache into single _symmetry_data tuple field - Add PEP 8 blank line between OrientationDict and RotationAnalyser - Add tests for _analyse_point_group outputs - Add brute-force equivalence regression test - Add tests for output shape, index consistency, input mutation - Add known-rotation test - Replace deprecated np.random.RandomState with default_rng
- Add Google-style docstring to polyhedron_orientation - Validate points shape in discrete_orientation - Add test for mismatched vertex count
This file contains hidden or 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
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.
Summary
RotationAnalyser.discrete_orientationwith a two-phase composition approach: find the best vertex alignment using symmetry-reduced permutations (via bsym), then compose the best-fit Procrustes rotation with the point group's proper rotation matrices to recover all orientation candidates.PointGroupAnalyzercall provides both the reduced permutations and the 3x3 rotation matrices, cached lazily on first access.OrientationDict.all_rotational_distancestype annotation (np.ndarray, notfloat).discrete_orientationcovering correct reference identification, inverted reference, and perfect geometry.assert isinstancechecks across the codebase.