Unify duplicated PBC correction logic into correct_pbc() - #41
Merged
Conversation
Extract a correct_pbc() utility in pbc_utils.py that encapsulates the unwrapping decision (reference centre vs legacy spread-based). Both PolyhedralSite and DynamicVoronoiSite now call this instead of duplicating the if/else block. Remove DynamicVoronoiSite._compute_corrected_coords entirely — calculate_centre and the batch fallback in the collection both call correct_pbc() directly, resolving the design smell where the method's primary effect was a side effect. PBC delegation tests consolidated in TestCorrectPbc; site-level tests verify correct_pbc is called with the right arguments.
- Fix missing trailing newlines in pbc_utils.py and test_pbc_utils.py - Replace convoluted assertion with simple kwargs check - Clarify lattice parameter docstrings in correct_pbc and _store_vertex_coords - Add type hints and align parameter order on _reference_centre test helper to match correct_pbc(frac_coords, reference_center, lattice) - Split int64 dtype test to cover both legacy and reference-centre paths - Add empty array guard in correct_pbc for zero-size input - Move test_reset_clears_centre into its own TestResetWithCalculatedCentre class
Move HAS_NUMBA into a dedicated _compat.py module as the single source of truth for optional dependency detection. Relocate update_pbc_shifts (numpy + numba variants) to pbc_utils.py where the other PBC utilities live. Absorb FaceTopologyCache and its numba helpers into polyhedral_site.py, their sole consumer. Delete containment.py and test_containment.py, with all tests moved to test_pbc_utils.py and test_polyhedral_site.py respectively.
Aligns the empty-array early return in unwrap_vertices_to_reference_center with the int64 dtype used consistently elsewhere in pbc_utils.
Fixes platform-dependent dtype in the image shifts returned by unwrap_vertices_to_reference_center, guaranteeing the int64 contract documented in correct_pbc on all platforms including Windows.
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
correct_pbc()utility function inpbc_utils.pythat encapsulates the PBC unwrapping decision (reference centre vs legacy spread-based), replacing character-for-character identical if/else blocks inPolyhedralSiteandDynamicVoronoiSiteDynamicVoronoiSite._compute_corrected_coordsentirely —calculate_centreand the batch fallback in the collection now callcorrect_pbc()directly, resolving the design smell where the method's primary effect was a side effectTestCorrectPbcintest_pbc_utils.py; site-level tests verifycorrect_pbcis called with the correct argumentscontainment.py: movesHAS_NUMBAto a dedicated_compat.pymodule, relocatesupdate_pbc_shifts(numpy + numba variants) topbc_utils.py, and absorbsFaceTopologyCachewith its numba helpers intopolyhedral_site.py(their sole consumer)test_containment.pytotest_pbc_utils.pyandtest_polyhedral_site.pyrespectively;containment.pywas internal (not exported from__init__.py) so there is no public API change