Repository: lace/polliwog

Issue:
Clean up matrix functions
- [x] Stop using `matrix` functions from vg (opened lace/vg#95 about removing them)
- [ ] Add `affine_transform`
- [x] Add `apply_affine_transform` (from `vg.matrix.transform`)

Discussion:

Patch:
diff --git a/polliwog/transform/affine_transform.py b/polliwog/transform/affine_transform.py
index 4c25c0d..feab638 100644
--- a/polliwog/transform/affine_transform.py
+++ b/polliwog/transform/affine_transform.py
@@ -18,3 +18,116 @@ def apply_affine_transform(points, transform_matrix):
     transformed_points = np.delete(transformed_padded_points, 3, axis=1)
 
     return maybe_decolumnize(transformed_points)
+
+
+def _convert_33_to_44(matrix):
+    """
+    Transform from:
+        array([[1., 2., 3.],
+               [2., 3., 4.],
+               [5., 6., 7.]])
+    to:
+        array([[1., 2., 3., 0.],
+               [2., 3., 4., 0.],
+               [5., 6., 7., 0.],
+               [0., 0., 0., 1.]])
+
+    """
+    vg.shape.check(locals(), "matrix", (3, 3))
+    result = np.pad(matrix, ((0, 1), (0, 1)), mode="constant")
+    result[3][3] = 1
+    return result
+
+
+def transform_matrix_for_rotation(rotation, ret_inverse_matrix=False):
+    """
+    Create a transformation matrix from the given 3x3 rotation matrix or a
+    Rodrigues vector.
+
+    With `ret_inverse_matrix=True`, also returns a matrix which provides
+    the reverse transform.
+    """
+    from .rodrigues import as_rotation_matrix
+
+    if rotation.shape == (3, 3):
+        forward3 = rotation
+    else:
+        vg.shape.check(locals(), "rotation", (3,))
+        forward3 = as_rotation_matrix(rotation)
+
+    forward = _convert_33_to_44(forward3)
+
+    if not ret_inverse_matrix:
+        return forward
+
+    # The inverse of a rotation matrix is its transpose.
+    inverse = forward.T
+    return forward, inverse
+
+
+def transform_matrix_for_translation(translation, ret_inverse_matrix=False):
+    """
+    Create a transformation matrix which translates by the provided
+    displacement vector.
+
+    Forward:
+
+        [[  1,  0,  0,  v_0 ],
+        [  0,  1,  0,  v_1 ],
+        [  0,  0,  1,  v_2 ],
+        [  0,  0,  0,  1   ]]
+
+    Reverse:
+
+        [[  1,  0,  0,  -v_0 ],
+        [  0,  1,  0,  -v_1 ],
+        [  0,  0,  1,  -v_2 ],
+        [  0,  0,  0,  1    ]]
+
+    Args:
+        vector (np.arraylike): A 3x1 vector.
+    """
+    vg.shape.check(locals(), "translation", (3,))
+
+    forward = np.eye(4)
+    forward[:, -1][:-1] = translation
+
+    if not ret_inverse_matrix:
+        return forward
+
+    inverse = np.eye(4)
+    inverse[:, -1][:-1] = -translation
+    return forward, inverse
+
+
+def transform_matrix_for_scale(scale_factor, ret_inverse_matrix=False):
+    """
+    Create a transformation matrix that scales by the given factor.
+
+    Forward:
+        [[  s_0, 0,   0,   0 ],
+        [  0,   s_1, 0,   0 ],
+        [  0,   0,   s_2, 0 ],
+        [  0,   0,   0,   1 ]]
+
+    Reverse:
+        [[  1/s_0, 0,     0,     0 ],
+        [  0,     1/s_1, 0,     0 ],
+        [  0,     0,     1/s_2, 0 ],
+        [  0,     0,     0,     1 ]]
+
+    Args:
+        factor (float): The scale factor.
+        ret_inverse_matrix (bool): When `True`, also returns a matrix which
+            provides the inverse transform.
+    """
+    if scale_factor <= 0:
+        raise ValueError("Scale factor should be greater than zero")
+
+    forward = _convert_33_to_44(np.diag(np.repeat(scale_factor, 3)))
+
+    if not ret_inverse_matrix:
+        return forward
+
+    inverse = _convert_33_to_44(np.diag(np.repeat(1.0 / scale_factor, 3)))
+    return forward, inverse
diff --git a/polliwog/transform/composite.py b/polliwog/transform/composite.py
index e0090d8..17a3c99 100644
--- a/polliwog/transform/composite.py
+++ b/polliwog/transform/composite.py
@@ -1,25 +1,5 @@
 import numpy as np
 import vg
-from .affine_transform import apply_affine_transform
-
-
-def _convert_33_to_44(matrix):
-    """
-    Transform from:
-        array([[1., 2., 3.],
-               [2., 3., 4.],
-               [5., 6., 7.]])
-    to:
-        array([[1., 2., 3., 0.],
-               [2., 3., 4., 0.],
-               [5., 6., 7., 0.],
-               [0., 0., 0., 1.]])
-
-    """
-    vg.shape.check(locals(), "matrix", (3, 3))
-    result = np.pad(matrix, ((0, 1), (0, 1)), mode="constant")
-    result[3][3] = 1
-    return result
 
 
 class CompositeTransform(object):
@@ -59,6 +39,8 @@ class CompositeTransform(object):
                 or reverse mode.
 
         """
+        from .affine_transform import apply_affine_transform
+
         transform_matrix = self.transform_matrix_for(
             from_range=from_range, reverse=reverse
         )
@@ -97,7 +79,7 @@ class CompositeTransform(object):
         matrix = reduce(np.dot, matrices)
         return matrix if reverse else matrix.T
 
-    def append_transform4(self, forward, reverse=None):
+    def append_transform(self, forward, reverse=None):
         """
         Append an arbitrary transformation, defined by 4x4 forward and reverse
         matrices.
@@ -105,56 +87,27 @@ class CompositeTransform(object):
         The new transformation is added to the end. Return its index.
 
         """
+        vg.shape.check(locals(), "forward", (4, 4))
         if reverse is None:
             reverse = np.linalg.inv(forward)
+        else:
+            vg.shape.check(locals(), "reverse", (4, 4))
 
         new_index = len(self.transforms)
         self.transforms.append((forward, reverse))
         return new_index
 
-    def append_transform3(self, forward, reverse=None):
-        """
-        Append an arbitrary transformation, defined by 3x3 forward and reverse
-        matrices.
-
-        The new transformation is added to the end. Return its index.
-
-        """
-        vg.shape.check(locals(), "forward", (3, 3))
-        forward4 = _convert_33_to_44(forward)
-        if reverse is None:
-            reverse4 = None
-        else:
-            vg.shape.check(locals(), "reverse", (3, 3))
-            reverse4 = _convert_33_to_44(reverse)
-        return self.append_transform4(forward4, reverse4)
-
     def scale(self, factor):
         """
         Scale by the given factor.
 
-        Forward:
-        [[  s_0, 0,   0,   0 ],
-        [  0,   s_1, 0,   0 ],
-        [  0,   0,   s_2, 0 ],
-        [  0,   0,   0,   1 ]]
-
-        Reverse:
-        [[  1/s_0, 0,     0,     0 ],
-        [  0,     1/s_1, 0,     0 ],
-        [  0,     0,     1/s_2, 0 ],
-        [  0,     0,     0,     1 ]]
-
         Args:
             factor (float): The scale factor.
         """
-        if factor <= 0:
-            raise ValueError("Scale factor should be greater than zero")
-
-        forward3 = np.diag(np.repeat(factor, 3))
-        reverse3 = np.diag(np.repeat(1.0 / factor, 3))
+        from .affine_transform import transform_matrix_for_scale
 
-        return self.append_transform3(forward3, reverse3)
+        forward, inverse = transform_matrix_for_scale(factor, ret_inverse_matrix=True)
+        return self.append_transform(forward, inverse)
 
     def convert_units(self, from_units, to_units):
         """
@@ -171,61 +124,37 @@ class CompositeTransform(object):
         import ounce
 
         factor = ounce.factor(from_units, to_units)
-        self.scale(factor)
+        return self.scale(factor)
 
     def translate(self, translation):
         """
         Translate by the vector provided.
 
-        Forward:
-
-            [[  1,  0,  0,  v_0 ],
-            [  0,  1,  0,  v_1 ],
-            [  0,  0,  1,  v_2 ],
-            [  0,  0,  0,  1   ]]
-
-        Reverse:
-
-            [[  1,  0,  0,  -v_0 ],
-            [  0,  1,  0,  -v_1 ],
-            [  0,  0,  1,  -v_2 ],
-            [  0,  0,  0,  1    ]]
-
         Args:
             vector (np.arraylike): A 3x1 vector.
         """
-        vg.shape.check(locals(), "translation", (3,))
+        from .affine_transform import transform_matrix_for_translation
 
-        forward = np.eye(4)
-        forward[:, -1][:-1] = translation
-
-        reverse = np.eye(4)
-        reverse[:, -1][:-1] = -translation
-
-        return self.append_transform4(forward, reverse)
+        forward, inverse = transform_matrix_for_translation(
+            translation, ret_inverse_matrix=True
+        )
+        return self.append_transform(forward, inverse)
 
     def reorient(self, up, look):
         """
         Reorient using up and look.
-
         """
         from .rotation import rotation_from_up_and_look
 
-        forward3 = rotation_from_up_and_look(up, look)
-        # The inverse of a rotation matrix is its transpose.
-        return self.append_transform3(forward3, forward3.T)
+        return self.rotate(rotation_from_up_and_look(up, look))
 
     def rotate(self, rotation):
         """
-        Rotate by either an explicit matrix or a rodrigues vector
+        Rotate by the given 3x3 rotation matrix or a Rodrigues vector.
         """
-        from .rodrigues import as_rotation_matrix
+        from .affine_transform import transform_matrix_for_rotation
 
-        if rotation.shape == (3, 3):
-            forward3 = rotation
-        else:
-            vg.shape.check(locals(), "rotation", (3,))
-            forward3 = as_rotation_matrix(rotation)
-
-        # The inverse of a rotation matrix is its transpose.
-        return self.append_transform3(forward3, forward3.T)
+        forward, inverse = transform_matrix_for_rotation(
+            rotation, ret_inverse_matrix=True
+        )
+        return self.append_transform(forward, inverse)
diff --git a/polliwog/transform/coordinate_manager.py b/polliwog/transform/coordinate_manager.py
index e301e8d..5761e56 100644
--- a/polliwog/transform/coordinate_manager.py
+++ b/polliwog/transform/coordinate_manager.py
@@ -33,11 +33,8 @@ class CoordinateManager(object):
             }
         )
 
-    def append_transform4(self, *args, **kwargs):
-        self._transform.append_transform4(*args, **kwargs)
-
-    def append_transform3(self, *args, **kwargs):
-        self._transform.append_transform3(*args, **kwargs)
+    def append_transform(self, *args, **kwargs):
+        self._transform.append_transform(*args, **kwargs)
 
     def scale(self, *args, **kwargs):
         self._transform.scale(*args, **kwargs)


Test patch:
diff --git a/polliwog/transform/test_affine_transform.py b/polliwog/transform/test_affine_transform.py
index f97d6ea..730089e 100644
--- a/polliwog/transform/test_affine_transform.py
+++ b/polliwog/transform/test_affine_transform.py
@@ -1,28 +1,105 @@
 import numpy as np
-from .affine_transform import apply_affine_transform
-
-scale_factor = np.array([3.0, 0.5, 2.0])
-transform = np.array(
-    [
-        [scale_factor[0], 0, 0, 0],
-        [0, scale_factor[1], 0, 0],
-        [0, 0, scale_factor[2], 0],
-        [0, 0, 0, 1],
-    ]
+import pytest
+from .affine_transform import (
+    apply_affine_transform,
+    transform_matrix_for_rotation,
+    transform_matrix_for_scale,
+    transform_matrix_for_translation,
 )
 
 
-def test_apply_homogeneous():
-    point = np.array([5.0, 0.0, 1.0])
-    expected_point = np.array([15.0, 0.0, 2.0])
-    np.testing.assert_array_equal(
-        apply_affine_transform(point, transform), expected_point
+def create_cube_verts(origin, size):
+    # Create a cube. Since CompositeTransform just works on verticies,
+    # we don't need a full lace.mesh object.
+    origin = np.array(origin)
+    size = np.repeat(size, 3)
+    lower_base_plane = np.array(
+        [
+            # Lower base plane
+            origin,
+            origin + np.array([size[0], 0, 0]),
+            origin + np.array([size[0], 0, size[2]]),
+            origin + np.array([0, 0, size[2]]),
+        ]
     )
+    upper_base_plane = lower_base_plane + np.array([0, size[1], 0])
+    return np.vstack([lower_base_plane, upper_base_plane])
+
+
+def create_default_cube_verts():
+    return create_cube_verts([1.0, 0.0, 0.0], 4.0)
 
 
-def test_apply_homogeneous_stacked():
+def test_apply_affine_transform():
+    scale_factor = np.array([3.0, 0.5, 2.0])
+    transform = np.array(
+        [
+            [scale_factor[0], 0, 0, 0],
+            [0, scale_factor[1], 0, 0],
+            [0, 0, scale_factor[2], 0],
+            [0, 0, 0, 1],
+        ]
+    )
+
     points = np.array([[1.0, 2.0, 3.0], [5.0, 0.0, 1.0]])
     expected_points = np.array([[3.0, 1.0, 6.0], [15.0, 0.0, 2.0]])
     np.testing.assert_array_equal(
         apply_affine_transform(points, transform), expected_points
     )
+    np.testing.assert_array_equal(
+        apply_affine_transform(points[1], transform), expected_points[1]
+    )
+
+
+def test_rotate():
+    cube_v = create_default_cube_verts()
+    ways_to_rotate_around_y_a_quarter_turn = [
+        np.array([[0, 0, 1], [0, 1, 0], [-1, 0, 0]]),
+        np.array([0, np.pi / 2, 0]),
+    ]
+    for rot in ways_to_rotate_around_y_a_quarter_turn:
+        # Confidence check.
+        np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
+        np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
+
+        transformed_cube_v = apply_affine_transform(
+            cube_v, transform_matrix_for_rotation(rot)
+        )
+
+        np.testing.assert_array_almost_equal(transformed_cube_v[0], [0.0, 0.0, -1.0])
+        np.testing.assert_array_almost_equal(transformed_cube_v[6], [4, 4.0, -5.0])
+
+
+def test_translate():
+    cube_v = create_default_cube_verts()
+
+    # Confidence check.
+    np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
+    np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
+
+    transformed_cube_v = apply_affine_transform(
+        cube_v, transform_matrix_for_translation(np.array([8.0, 6.0, 7.0]))
+    )
+
+    np.testing.assert_array_equal(transformed_cube_v[0], [9.0, 6.0, 7.0])
+    np.testing.assert_array_equal(transformed_cube_v[6], [13.0, 10.0, 11.0])
+
+
+def test_scale():
+    cube_v = create_default_cube_verts()
+
+    # Confidence check.
+    np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
+    np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
+
+    transformed_cube_v = apply_affine_transform(
+        cube_v, transform_matrix_for_scale(10.0)
+    )
+
+    np.testing.assert_array_equal(transformed_cube_v[0], [10.0, 0.0, 0.0])
+    np.testing.assert_array_equal(transformed_cube_v[6], [50.0, 40.0, 40.0])
+
+
+def test_scale_error():
+    with pytest.raises(ValueError, match="Scale factor should be greater than zero"):
+        transform_matrix_for_scale(-1)
diff --git a/polliwog/transform/test_composite.py b/polliwog/transform/test_composite.py
index 53569b5..a6fe883 100644
--- a/polliwog/transform/test_composite.py
+++ b/polliwog/transform/test_composite.py
@@ -1,67 +1,7 @@
 import numpy as np
-import pytest
 import vg
 from .composite import CompositeTransform
-
-
-def create_cube_verts(origin, size):
-    # Create a cube. Since CompositeTransform just works on verticies,
-    # we don't need a full lace.mesh object.
-    origin = np.asarray(origin)
-    size = np.repeat(size, 3)
-    lower_base_plane = np.array(
-        [
-            # Lower base plane
-            origin,
-            origin + np.array([size[0], 0, 0]),
-            origin + np.array([size[0], 0, size[2]]),
-            origin + np.array([0, 0, size[2]]),
-        ]
-    )
-    upper_base_plane = lower_base_plane + np.array([0, size[1], 0])
-    return np.vstack([lower_base_plane, upper_base_plane])
-
-
-def create_default_cube_verts():
-    return create_cube_verts([1.0, 0.0, 0.0], 4.0)
-
-
-def test_translate():
-    transform = CompositeTransform()
-    transform.translate(np.array([8.0, 6.0, 7.0]))
-
-    cube_v = create_default_cube_verts()
-
-    # Confidence check.
-    np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
-    np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
-
-    transformed_cube_v = transform(cube_v)
-
-    np.testing.assert_array_equal(transformed_cube_v[0], [9.0, 6.0, 7.0])
-    np.testing.assert_array_equal(transformed_cube_v[6], [13.0, 10.0, 11.0])
-
-
-def test_scale():
-    transform = CompositeTransform()
-    transform.scale(10.0)
-
-    cube_v = create_default_cube_verts()
-
-    # Confidence check.
-    np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
-    np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
-
-    transformed_cube_v = transform(cube_v)
-
-    np.testing.assert_array_equal(transformed_cube_v[0], [10.0, 0.0, 0.0])
-    np.testing.assert_array_equal(transformed_cube_v[6], [50.0, 40.0, 40.0])
-
-
-def test_scale_error():
-    transform = CompositeTransform()
-    with pytest.raises(ValueError):
-        transform.scale(-1)
+from .test_affine_transform import create_default_cube_verts
 
 
 def test_convert_units():
@@ -150,26 +90,6 @@ def test_reorient():
     np.testing.assert_array_equal(transformed_cube_v[6], [4, 4.0, -5.0])
 
 
-def test_rotate():
-    ways_to_rotate_around_y_a_quarter_turn = [
-        np.array([[0, 0, 1], [0, 1, 0], [-1, 0, 0]]),
-        np.array([0, np.pi / 2, 0]),
-    ]
-    for rot in ways_to_rotate_around_y_a_quarter_turn:
-        transform = CompositeTransform()
-        transform.rotate(rot)
-        cube_v = create_default_cube_verts()
-
-        # Confidence check.
-        np.testing.assert_array_equal(cube_v[0], [1.0, 0.0, 0.0])
-        np.testing.assert_array_equal(cube_v[6], [5.0, 4.0, 4.0])
-
-        transformed_cube_v = transform(cube_v)
-
-        np.testing.assert_array_almost_equal(transformed_cube_v[0], [0.0, 0.0, -1.0])
-        np.testing.assert_array_almost_equal(transformed_cube_v[6], [4, 4.0, -5.0])
-
-
 def test_reverse_transforms():
     transforms = [CompositeTransform() for _ in range(5)]
 
diff --git a/polliwog/transform/test_coordinate_manager.py b/polliwog/transform/test_coordinate_manager.py
index 3cecb81..414f2f0 100644
--- a/polliwog/transform/test_coordinate_manager.py
+++ b/polliwog/transform/test_coordinate_manager.py
@@ -2,7 +2,7 @@ import numpy as np
 import pytest
 import vg
 from .coordinate_manager import CoordinateManager
-from .test_composite import create_cube_verts
+from .test_affine_transform import create_cube_verts
 
 
 def perform_transform_test(apply_transform_fn, expected_v0, expected_v6):
@@ -132,19 +132,10 @@ def test_coordinate_manager_invalid_tag():
 
 
 def test_coordinate_manager_custom_transform():
-    scale4 = np.array([[3, 0, 0, 0], [0, 3, 0, 0], [0, 0, 3, 0], [0, 0, 0, 1]])
+    scale = np.array([[3, 0, 0, 0], [0, 3, 0, 0], [0, 0, 3, 0], [0, 0, 0, 1]])
     perform_transform_test(
-        apply_transform_fn=lambda coordinate_manager: coordinate_manager.append_transform4(
-            scale4
-        ),
-        expected_v0=np.array([3.0, 0.0, 0.0]),
-        expected_v6=np.array([15.0, 12.0, 12.0]),
-    )
-
-    scale3 = np.array([[3, 0, 0], [0, 3, 0], [0, 0, 3]])
-    perform_transform_test(
-        apply_transform_fn=lambda coordinate_manager: coordinate_manager.append_transform3(
-            scale3
+        apply_transform_fn=lambda coordinate_manager: coordinate_manager.append_transform(
+            scale
         ),
         expected_v0=np.array([3.0, 0.0, 0.0]),
         expected_v6=np.array([15.0, 12.0, 12.0]),