deprecate: UnitQuaternion.interp1's shortest parameter is a no-op

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 scalar part. The negative-dot-product condition
`shortest` checks for to detect "the long way round" can therefore
never occur here -- confirmed empirically (2000 random compositions,
plus explicit UnitQuaternion([-1,0,0,0]) construction, all canonicalize
to non-negative scalar part).

Drops the dead branch, warns on shortest=True, documents why in a
docstring note, and pins the no-op behavior (identical results for
True/False, warns only when True) with a regression test.

Author: petercorke

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
@@ -1978,7 +1979,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
@@ -2009,10 +2010,32 @@ 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.17
+            ``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:
@@ -2022,14 +2045,6 @@ def interp1(self, s: float = 0, shortest: Optional[bool] = False) -> UnitQuatern
         q = self.vec
         dot = q[0]  # s
 
-        # If the dot product is negative, the quaternions
-        # have opposite handed-ness and slerp won't take
-        # the shorter path. Fix by reversing one quaternion.
-        if shortest:
-            if dot < 0:
-                q = -q
-                dot = -dot
-
         # shouldn't be needed by handle numerical errors: -eps, 1+eps cases
         dot = np.clip(dot, -1, 1)  # Clip within domain of acos()
 

tests/test_quaternion.py

@@ -2,6 +2,7 @@
 from math import pi
 import numpy.testing as nt
 import unittest
+import warnings
 
 from spatialmath import *
 from spatialmath.base import *
@@ -701,6 +702,31 @@ def test_interp(self):
         # qcompare( qq(6), UnitQuaternion.Rx(pi) )
         # TODO interp
 
+    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()