Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix quaternion comparison #8

Merged
merged 24 commits into from
Nov 24, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion quaternions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
from quaternions.quaternions import Quaternion, QuaternionError # NOQA
from quaternions.quaternions import Quaternion # NOQA
from quaternions.general_quaternion import GeneralQuaternion, QuaternionError # NOQA
from quaternions.version import __version__ # NOQA
185 changes: 185 additions & 0 deletions quaternions/general_quaternion.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
import numpy as np


class QuaternionError(Exception):
pass


DEFAULT_TOLERANCE = 1e-8


class GeneralQuaternion(object):
"""
represents a quaternion, considered a member in Lie algebra of quaternions.
for backward compatibility shares the same interface with Quaternion.
unlike quaternion, it's not defined up to scale
"""
def __init__(self, qr, qi, qj, qk):
self.qr = qr
self.qi = qi
self.qj = qj
self.qk = qk

def __add__(self, p):
if not is_quaternion(p):
raise QuaternionError('expected quaternion, got %s' % p)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the message would be more clear would it be
'expected quaternion, got %s' % type(p)
or even
'expected quaternion, got %s' % p.__class__.__name__

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return self.__class__(self.qr + p.qr, self.qi + p.qi, self.qj + p.qj, self.qk + p.qk)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sum of two quaternions should be a GeneralQuaternion.
Same thing for __sub__

Notice that with this definition, sum would not commute:

In [13]: p
Out[13]: GeneralQuaternion[ 0.18257419  0.36514837  0.54772256  0.73029674]  # <-- p is actually a Quaternion

In [14]: q
Out[14]: GeneralQuaternion[2 3 4 5]

In [15]: p + q
Out[15]: GeneralQuaternion[ 0.2616049   0.40334909  0.54509328  0.68683747]

In [16]: q + p
Out[16]: GeneralQuaternion[ 2.18257419  3.36514837  4.54772256  5.73029674]

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


def __sub__(self, p):
if not is_quaternion(p):
raise QuaternionError('expected quaternion, got %s' % p)
return self.__class__(self.qr - p.qr, self.qi - p.qi, self.qj - p.qj, self.qk - p.qk)

def __neg__(self):
return self.__class__(-self.qr, -self.qi, -self.qj, -self.qk)

def __mul__(self, p):
if is_quaternion(p):
mat = np.array([
[self.qr, -self.qi, -self.qj, -self.qk], # noqa
[self.qi, self.qr, self.qk, -self.qj], # noqa
[self.qj, -self.qk, self.qr, self.qi], # noqa
[self.qk, self.qj, -self.qi, self.qr] # noqa
])
result = mat.dot(np.array(p.coordinates))
return self.__class__(*result)
else:
return self.__class__(self.qr * p, self.qi * p, self.qj * p, self.qk * p)

def __rmul__(self, p):
return self.__mul__(p)

def __truediv__(self, p):
return self * (1 / p)

def __rtruediv__(self, p):
return p * self.inverse()

def conjugate(self):
return self.__class__(self.qr, -self.qi, -self.qj, -self.qk)

def inverse(self):
return self.conjugate() * (1 / self._squarenorm())

def __invert__(self):
return self.inverse()

def _squarenorm(self):
return self.qr * self.qr + self.qi * self.qi + self.qj * self.qj + self.qk * self.qk

def __repr__(self):
return 'GeneralQuaternion{}'.format(self.coordinates)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now self.coordinates is a np.array, it would be better to convert them to tuple here:
'GeneralQuaternion{}'.format(tuple(self.coordinates))

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


def __str__(self):
return '({qr:.6g}{qi:+.6g}i{qj:+.6g}j{qk:+.6g}k)'.format(**self.__dict__)

def is_real(self, tolerance=DEFAULT_TOLERANCE):
""" True if i, j, k components are zero. """
complex_norm = np.linalg.norm([self.qi, self.qj, self.qk])
return complex_norm < tolerance

def is_equal(self, other, tolerance=DEFAULT_TOLERANCE):
"""
compares as quaternions up to tolerance.
Note: tolerance in coords, not in quaternions metrics.
Note: unlike quaternions, equality is not up to scale.
"""
return np.linalg.norm(self.coordinates - other.coordinates) < tolerance

def __eq__(self, other):
return self.is_equal(other)

def norm(self):
return np.sqrt(self._squarenorm())

def normalized(self):
return self / self.norm()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a Quaternion instead of a GeneralQuaternion

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wasn't sure about it. i didn't want GeneralQuaternion to know about derived class, it's cyclic. what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, it is not the best thing to do.
Actually, the constructor for Quaternion now normalizes quaternions, so one could do
Quaternion(*q.coordinates) and get the normalized quaternion with the unitary class.

Ok, let's leave it like this. We could add a comment about how to get a Quaternion instead of a GeneralQuaternion here.


def distance(self, other):
""" Returns the distance between two unitary quaternions """
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not unitary and the concept of distance is different from the one we used for Quaternions.
I'd rather not add this method here and leave it only in Quaternion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, unitary is typo.

but i think we need distance for GeneralQuaternion too, no? (although i have no use-case in mind)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might help to have such a method.
What do you think about euclidean_distance for this one?

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return (self - other).norm()

def is_unitary(self, tolerance=DEFAULT_TOLERANCE):
return abs(self.norm() - 1) < tolerance

@property
def coordinates(self):
return np.array([self.qr, self.qi, self.qj, self.qk])

@property
def basis(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method makes sense for unitary quaternions and it's actually kept for backwards compatibility.
I'd take it out from here.

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

qr, qi, qj, qk = self.coordinates
b0 = np.array([
qr ** 2 + qi ** 2 - qj ** 2 - qk ** 2,
2 * qr * qk + 2 * qi * qj,
-2 * qr * qj + 2 * qi * qk
])
b1 = np.array([
-2 * qr * qk + 2 * qi * qj,
qr ** 2 - qi ** 2 + qj ** 2 - qk ** 2,
2 * qr * qi + 2 * qj * qk
])
b2 = np.array([
2 * qr * qj + 2 * qi * qk,
-2 * qr * qi + 2 * qj * qk,
qr ** 2 - qi ** 2 - qj ** 2 + qk ** 2
])
return b0, b1, b2

@classmethod
def _first_eigenvector(cls, matrix):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this method to Quaternion, together with average

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

""" matrix must be a 4x4 symmetric matrix. """
vals, vecs = np.linalg.eigh(matrix)
# q is the eigenvec with heighest eigenvalue (already normalized)
q = vecs[:, -1]
if q[0] < 0:
q = -q
return cls(*q)

@staticmethod
def average(*quaternions, weights=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method should return a Quaternion. Also, I think I'd leave the whole method in Quaternion, as it solves a problem pertaining to unitary quaternions and not general ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

average makes sense for GeneralQ. too, no?

but i agree, Quaternion.average() should return Quaternion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

average in the Lie algebra would be the "usual" average, something like
GeneralQuaternion(np.sum(q.coordinates for q in quaternions)) / len(quaternions).
That average would be the one that minimizes the sum of squares of euclidean distances to all elements in quaternions.
The Quaternion.average minimizes the sum of squares of distances of 3x3 matrices, which is closer to the concept of distance in that class.

Actually, I'm a bit puzzled now, as there should be an average minimizing squares of distances in the sense of .log().norm(). I don't know what it is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when needed, we can implement GeneralQuaternion.average() as the euclidean average

"""
Return the quaternion such that its matrix minimizes the square distance
to the matrices of the quaternions in the argument list.

See Averaging Quaternions, by Markley, Cheng, Crassidis, Oschman.
"""
b = np.array([q.coordinates for q in quaternions])
if weights is None:
weights = np.ones(len(quaternions))
m = b.T.dot(np.diag(weights)).dot(b)

return GeneralQuaternion._first_eigenvector(m)

@classmethod
def unit(cls):
return cls(1, 0, 0, 0)

@classmethod
def zero(cls):
return cls(0, 0, 0, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should be GeneralQuaternion(0, 0, 0, 0)

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


@classmethod
def exp(cls, q):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to keep an instance method like

def exp(self):
    return GeneralQuaternion.exp(self)

Copy link
Author

@slava-kerner slava-kerner Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"""
exponent quaternion
:param q: list of 4 items or Quaternion (or any derived)
:return: Quaternion (or any derived)
"""
if is_quaternion(q):
real, imag = q.coordinates[0], q.coordinates[1:]
else:
real, imag = q[0], np.asarray(q[1:])

exp_norm = np.exp(real)

imag_norm = np.linalg.norm(imag)
if imag_norm == 0:
return cls(exp_norm, 0, 0, 0)

j, k, l = np.sin(imag_norm) * imag / imag_norm
return exp_norm * cls(np.cos(imag_norm), j, k, l)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if real == 0 we can return a Quaternion here.
Anyway, maybe the best thing to do is to add in Quaternion a method like

@classmethod
def exp(cls, q):
    assert np.abs(q.qr) < DEFAULT_TOLERANCE, 'Real part is nonzero. Use GeneralQuaternion.exp`

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i understand.
exp(Quaternion) is valid even if Quaternion has nonzero real part, no?

and i'd prefer exp(GeneralQuaternion) to always return GeneralQuaternion, i wouldn't want return type dependant on values.
one can always convert GeneralQuaternion to Quaternion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the way you did it.
But I'd like that

In [19]: Quaternion.exp(GeneralQuaternion(0,2,3,4))
Out[19]: GeneralQuaternion(0.62315934497621972, -0.29046275344788247, -0.43569413017182368, -0.58092550689576494)

would return a Quaternion and raise an exception if the real part of the original quaternion is nonzero.



def is_quaternion(q):
return issubclass(type(q), GeneralQuaternion)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason not to use
isinstance(q, GeneralQuaternion)?
Isinstance seems more natural to me. Anyway, this might just be one of those subjective differences; no need to change it if you prefer this form.

Loading