Merge PR #209: deprecate: UnitQuaternion.interp1's shortest parameter is a no-op

Author: taughz

spatialmath/quaternion.py

@@ -16,6 +16,7 @@
 # pylint: disable=invalid-name
 from __future__ import annotations
 import math
+import warnings
 import numpy as np
 from typing import Any
 import spatialmath.base as smb
@@ -2073,7 +2074,7 @@ def interp1(self, s: float = 0, shortest: Optional[bool] = False) -> UnitQuatern
         """
         Interpolate a unit quaternion
 
-        :param shortest: Take the shortest path along the great circle
+        :param shortest: deprecated, has no effect
         :param s: interpolation coefficient, range 0 to 1, or number of steps
         :type s: array_like or int
         :return: interpolated unit quaternion
@@ -2104,19 +2105,39 @@ def interp1(self, s: float = 0, shortest: Optional[bool] = False) -> UnitQuatern
 
         .. note:: values of ``s`` are silently clipped to the range [0, 1]
 
+        .. note:: ``shortest`` can never change the result here. Every
+            ``UnitQuaternion`` has a non-negative scalar part by construction
+            (see :func:`~spatialmath.base.quaternions.qunit`), and this method
+            always interpolates *from* the identity quaternion ``[1,0,0,0]``
+            -- whose dot product with any unit quaternion is just that
+            quaternion's own (always non-negative) scalar part. A negative
+            dot product is what ``shortest`` checks for to detect "the long
+            way round", and that condition can never occur here, unlike
+            :meth:`interp`, where it can.
+
+        .. deprecated:: 1.1.19
+            ``shortest`` has no effect and will be removed in a future
+            release.
+
         :seealso: :func:`~spatialmath.base.quaternions.qslerp`
         """
         # TODO allow self to have len() > 1
 
+        if shortest:
+            warnings.warn(
+                "shortest has no effect on interp1 and will be removed in a "
+                "future release: interpolation from the identity quaternion "
+                "is always the shortest path",
+                DeprecationWarning,
+            )
+
         if isinstance(s, int) and s > 1:
             s = np.linspace(0, 1, s)
         else:
             s = smb.getvector(s)
             s = np.clip(s, 0, 1)  # enforce valid values
 
-        q0_endpoint, q0, q1, sin_theta, theta = _qslerp_prepare(
-            smb.qeye(), self.vec, shortest=shortest
-        )
+        q0_endpoint, q0, q1, sin_theta, theta = _qslerp_prepare(smb.qeye(), self.vec)
         qi = []
         for sk in s:
             if sk == 0:

tests/test_quaternion.py

@@ -3,6 +3,7 @@
 import numpy.testing as nt
 import unittest
 from unittest.mock import patch
+import warnings
 
 from spatialmath import *
 from spatialmath.base import *
@@ -783,6 +784,31 @@ def test_interp_prepares_slerp_once(self):
                 self.assertEqual(len(interpolate()), 5)
             self.assertEqual(dot.call_count, 1)
 
+    def test_interp1_shortest_deprecated(self):
+        # shortest never had an effect on interp1: every UnitQuaternion has
+        # a non-negative scalar part by construction (qunit()), and interp1
+        # always interpolates from the identity quaternion, whose dot
+        # product with any unit quaternion is just that quaternion's own
+        # (always non-negative) scalar part. So the "long way round" branch
+        # shortest exists to avoid is unreachable. Pin that True/False give
+        # identical results, that it warns, and that leaving it at its
+        # default doesn't.
+        q = UnitQuaternion.Rx(4.5)  # would be "the long way" if reachable
+
+        for s in (0, 0.25, 0.5, 0.75, 1):
+            qcompare(q.interp1(s, shortest=False), q.interp1(s, shortest=True))
+
+        for a, b in zip(q.interp1(11, shortest=False), q.interp1(11, shortest=True)):
+            qcompare(a, b)
+
+        with self.assertWarns(DeprecationWarning):
+            q.interp1(0.5, shortest=True)
+
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            q.interp1(0.5)  # default shortest=False must not warn
+            q.interp1(0.5, shortest=False)
+
     def test_increment(self):
         q = UnitQuaternion()