From f4e2838d151cac21e2a5b7987e89ced3e38448d5 Mon Sep 17 00:00:00 2001 From: "Haoyu (Daniel) YANG" Date: Tue, 22 Oct 2024 04:54:56 +0800 Subject: [PATCH] Fix arg passing in `inverse` property of `SymmOp` (#4113) * fix arg passing * add more unit test * perhaps overwrite? not sure which is better * what about making a copy? * make sure copy is correctly done * use np.asarray to avoid copying * remove unnecessary id check now that we're deep copy * try to bump networkx to 3.0+ * Revert "try to bump networkx to 3.0+" This reverts commit b27adbacd892e51df6bc27488126a5742177c639. --- src/pymatgen/core/operations.py | 33 +++++++++++++++++++-------------- tests/core/test_operations.py | 16 ++++++++++++++-- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/pymatgen/core/operations.py b/src/pymatgen/core/operations.py index c83c3c2161f..d54bd95a432 100644 --- a/src/pymatgen/core/operations.py +++ b/src/pymatgen/core/operations.py @@ -2,6 +2,7 @@ from __future__ import annotations +import copy import re import string import warnings @@ -51,7 +52,7 @@ def __init__( Raises: ValueError: if matrix is not 4x4. """ - affine_transformation_matrix = np.array(affine_transformation_matrix) + affine_transformation_matrix = np.asarray(affine_transformation_matrix) shape = affine_transformation_matrix.shape if shape != (4, 4): raise ValueError(f"Affine Matrix must be a 4x4 numpy array, got {shape=}") @@ -96,8 +97,8 @@ def from_rotation_and_translation( Returns: SymmOp object """ - rotation_matrix = np.array(rotation_matrix) - translation_vec = np.array(translation_vec) + rotation_matrix = np.asarray(rotation_matrix) + translation_vec = np.asarray(translation_vec) if rotation_matrix.shape != (3, 3): raise ValueError("Rotation Matrix must be a 3x3 numpy array.") if translation_vec.shape != (3,): @@ -117,7 +118,7 @@ def operate(self, point: ArrayLike) -> np.ndarray: Returns: Coordinates of point after operation. """ - affine_point = np.array([*point, 1]) + affine_point = np.asarray([*point, 1]) return np.dot(self.affine_matrix, affine_point)[:3] def operate_multi(self, points: ArrayLike) -> np.ndarray: @@ -129,7 +130,7 @@ def operate_multi(self, points: ArrayLike) -> np.ndarray: Returns: Numpy array of coordinates after operation """ - points = np.array(points) + points = np.asarray(points) affine_points = np.concatenate([points, np.ones(points.shape[:-1] + (1,))], axis=-1) return np.inner(affine_points, self.affine_matrix)[..., :-1] @@ -243,12 +244,16 @@ def translation_vector(self) -> np.ndarray: @property def inverse(self) -> Self: """Inverse of transformation.""" - inverse = np.linalg.inv(self.affine_matrix) - return type(self)(inverse) + new_instance = copy.deepcopy(self) + new_instance.affine_matrix = np.linalg.inv(self.affine_matrix) + return new_instance @staticmethod def from_axis_angle_and_translation( - axis: ArrayLike, angle: float, angle_in_radians: bool = False, translation_vec: ArrayLike = (0, 0, 0) + axis: ArrayLike, + angle: float, + angle_in_radians: bool = False, + translation_vec: ArrayLike = (0, 0, 0), ) -> SymmOp: """Generate a SymmOp for a rotation about a given axis plus translation. @@ -266,7 +271,7 @@ def from_axis_angle_and_translation( if isinstance(axis, tuple | list): axis = np.array(axis) - vec = np.array(translation_vec) + vec = np.asarray(translation_vec) ang = angle if angle_in_radians else angle * pi / 180 cos_a = cos(ang) @@ -368,7 +373,7 @@ def reflection(normal: ArrayLike, origin: ArrayLike = (0, 0, 0)) -> SymmOp: u, v, w = normal translation = np.eye(4) - translation[:3, 3] = -np.array(origin) + translation[:3, 3] = -np.asarray(origin) xx = 1 - 2 * u**2 yy = 1 - 2 * v**2 @@ -395,7 +400,7 @@ def inversion(origin: ArrayLike = (0, 0, 0)) -> SymmOp: """ mat = -np.eye(4) mat[3, 3] = 1 - mat[:3, 3] = 2 * np.array(origin) + mat[:3, 3] = 2 * np.asarray(origin) return SymmOp(mat) @staticmethod @@ -505,11 +510,11 @@ def __init__( tol (float): Tolerance for determining if matrices are equal. """ super().__init__(affine_transformation_matrix, tol=tol) - if time_reversal in {-1, 1}: - self.time_reversal = time_reversal - else: + if time_reversal not in {-1, 1}: raise RuntimeError(f"Invalid {time_reversal=}, must be 1 or -1") + self.time_reversal = time_reversal + def __eq__(self, other: object) -> bool: if not isinstance(other, type(self)): return NotImplemented diff --git a/tests/core/test_operations.py b/tests/core/test_operations.py index ad4a1ccd1e1..adf5f9d0b63 100644 --- a/tests/core/test_operations.py +++ b/tests/core/test_operations.py @@ -33,7 +33,12 @@ def test_operate_multi(self): def test_inverse(self): point = np.random.default_rng().random(3) new_coord = self.op.operate(point) - assert_allclose(self.op.inverse.operate(new_coord), point, 2) + + # Make sure tol is passed correctly + self.op.tol = 0.02 # non-default + inverse_op = self.op.inverse + assert_allclose(inverse_op.operate(new_coord), point, 2) + assert_allclose(self.op.tol, inverse_op.tol) def test_reflection(self): rng = np.random.default_rng() @@ -237,7 +242,6 @@ def test_xyzt_string(self): magop = MagSymmOp.from_symmop(op, -1) magop_str = magop.as_xyzt_str() - assert magop.time_reversal == -1 assert magop_str == "3x-2y-z+1/2, -x+12/13, z+1/2, -1" def test_as_from_dict(self): @@ -263,3 +267,11 @@ def test_operate_magmom(self): for magmom in magmoms: op = MagSymmOp.from_xyzt_str(xyzt_string) assert_allclose(transformed_magmom, op.operate_magmom(magmom).global_moment) + + def test_inverse(self): + op = SymmOp([[3, -2, -1, 0.5], [-1, 0, 0, 12.0 / 13], [0, 0, 1, 0.5 + 1e-7], [0, 0, 0, 1]], tol=0.02) + + magop = MagSymmOp.from_symmop(op, -1) + assert magop.time_reversal == -1 + assert magop.tol == 0.02 + assert_allclose(magop.inverse.affine_matrix, np.linalg.inv(magop.affine_matrix))