Fix quaternion comparison #8

slava-kerner · 2017-11-16T07:22:43Z

Quaternion.log() now returns np.array, and not Quaternion - since log of a quaternion is not defined up to scale, and can be [0, 0, 0, 0].
it actually resides in lie algebra, not in the lie group.
respectively Quaternion.exp() can now take also np.array, to enable Quaternion.exp(0, 0, 0, 0)

this fixes problems of numerically unstable quaternions (where log(identity)=0)

2) Quaternion.exp() takes np.array

coveralls · 2017-11-16T07:58:58Z

Coverage increased (+0.1%) to 90.286% when pulling 4b960ff on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T07:58:58Z

Coverage increased (+0.1%) to 90.286% when pulling 4b960ff on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T07:58:58Z

Coverage increased (+0.1%) to 90.286% when pulling 4b960ff on fix_quaternion_comparison into 28dba2a on master.

…assignment target"

coveralls · 2017-11-16T08:05:54Z

Coverage increased (+0.1%) to 90.286% when pulling 75f583c on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T08:05:54Z

Coverage increased (+0.1%) to 90.286% when pulling 75f583c on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T08:05:54Z

Coverage increased (+0.1%) to 90.286% when pulling 75f583c on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T08:05:54Z

Coverage increased (+0.1%) to 90.286% when pulling 75f583c on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-16T08:09:46Z

Coverage increased (+0.1%) to 90.286% when pulling 0af64dd on fix_quaternion_comparison into 28dba2a on master.

astrojuanlu

Just added a couple of general comments - cannot say anything about the mathematical part.

astrojuanlu · 2017-11-16T09:54:40Z

LICENSE.txt

@@ -0,0 +1 @@
+Satellogic SA Copyright 2017. All our code is GPLv3 licensed.


This is not needed: https://github.com/satellogic/quaternions/blob/master/LICENSE

oops spillover, will remove

astrojuanlu · 2017-11-16T09:55:00Z

setup.cfg

@@ -0,0 +1,2 @@
+[metadata]
+description-file = README.md


Missing newline

oops spillover, is it needed?? @Juanlu001

Not mandatory, but when doing $ cat setup.cfg you will see the last line merged with your prompt. Just aesthetic details :)

no, i mean - i added setup.cfg half-accidentally, spillover from some other project.
do we want this file here?

astrojuanlu · 2017-11-16T09:55:23Z

quaternions/version.py

@@ -1,2 +1,2 @@
 # https://www.python.org/dev/peps/pep-0440/
-__version__ = '0.1.3.dev0'
+__version__ = '0.1.3.dev1'


I'm sure @matiasg wants to release a stable version sooner than later :)

yep, i think there are 2 more PRs pending, after which 0.1.4?

astrojuanlu · 2017-11-16T10:02:57Z

Oh, in this case I'd remove it - not sure what's the effect of that line.

…

On Nov 16, 2017 11:01, "slava-kerner" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In setup.cfg <#8 (comment)>: > @@ -0,0 +1,2 @@ +[metadata] +description-file = README.md no, i mean - i added setup.cfg half-accidentally, spillover from some other project. do we want this file here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATUZbOdB1zNc87FX3n9E6B3lsX7yHlJks5s3AfogaJpZM4QgD_W> .

coveralls · 2017-11-16T10:10:05Z

Coverage increased (+0.1%) to 90.286% when pulling 38495b6 on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-21T05:05:05Z

Coverage increased (+1.3%) to 91.429% when pulling 17a258f on fix_quaternion_comparison into 28dba2a on master.

matiasg

I like the whole thing a lot. There are a few things to settle down though (like, for instance, where average method belongs to).

matiasg · 2017-11-21T12:31:11Z

quaternions/general_quaternion.py

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


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__

matiasg · 2017-11-21T12:32:31Z

quaternions/general_quaternion.py

+    def __add__(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)


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]

matiasg · 2017-11-21T12:51:04Z

quaternions/general_quaternion.py

+
+
+def is_quaternion(q):
+    return issubclass(type(q), GeneralQuaternion)


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.

matiasg · 2017-11-21T13:46:56Z

quaternions/general_quaternion.py

+        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)


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

matiasg · 2017-11-21T14:40:10Z

setup.py

@@ -12,7 +12,7 @@
 setup(
    name='satellogic_quaternions',
    version=version["__version__"],
-    author='Matias Graña',
+    author='Matias Graña, Enrique Toomey, Slava Kerner',


👍
Maybe we have to change the email accordingly.

matiasg · 2017-11-21T15:42:02Z

quaternions/quaternions.py

-    def inverse(self):
-        return self.conjugate() * (1 / self._squarenorm())
+    def __call__(self, p):
+        return self * p


This is an interesting feature :-) I'm not sure people would use the function syntax for anything else than multiplying by a 3-dimensional vector. Maybe, I'd add an assert isinstance(p, Iterable) and len(p) == 3, or something like that, just to prevent an unintended incorrect use.

matiasg · 2017-11-21T15:43:48Z

quaternions/quaternions.py


-    def is_unitary(self):
-        return abs(self._squarenorm() - 1) < self.tolerance
+        j, k, l = imag / imag_norm * np.arctan2(imag_norm, self.qr / norm)


why not i, j, k instead of j, k, l?

matiasg · 2017-11-21T15:46:58Z

tests/test_general_quaternion.py

+from quaternions.general_quaternion import GeneralQuaternion, QuaternionError, DEFAULT_TOLERANCE
+
+
+ANY_QUATERNION = strategies.lists(elements=strategies.floats(min_value=-5, max_value=5), min_size=4, max_size=4)


awesome! :-)

matiasg · 2017-11-21T15:51:09Z

tests/test_general_quaternion.py

+        assert q + q == 2 * q == q * 2
+        assert q - q == GeneralQuaternion.zero()
+        assert q * GeneralQuaternion.zero() == q * 0 == GeneralQuaternion.zero()
+        assert q * GeneralQuaternion.unit() == q * 1 == q


I'd add a line with the multiplications in the other order:

assert GeneralQuaternion.unit() * q == 1 * q == q

matiasg · 2017-11-21T15:52:48Z

tests/test_general_quaternion.py

+    @given(ANY_QUATERNION)
+    def test_conjugate(self, arr):
+        q = GeneralQuaternion(*arr)
+        assert (q + q.conjugate()).is_real()


😮 github allows to edit other's comments.
Anyway, it's fixed 😉

coveralls · 2017-11-22T11:18:12Z

Coverage increased (+5.7%) to 95.89% when pulling ff0e95e on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-22T11:18:12Z

Coverage increased (+5.7%) to 95.89% when pulling ff0e95e on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-22T11:22:43Z

Coverage increased (+5.7%) to 95.89% when pulling 15329d0 on fix_quaternion_comparison into 28dba2a on master.

coveralls · 2017-11-22T11:32:29Z

Coverage increased (+5.7%) to 95.89% when pulling 932d7e9 on fix_quaternion_comparison into 28dba2a on master.

matiasg · 2017-11-24T19:27:41Z

👍

# This is the 1st commit message: pin coveralls version # This is the commit message #2: rever change of pytest-coverage. Pin python-coverage # This is the commit message #3: just as a test, remove coveralls # This is the commit message #4: remove also cov from script 🤦 # This is the commit message #5: pin pytest version # This is the commit message #6: pin hypothesis version # This is the commit message #7: pin hypothesis to 3.55.1 now # This is the commit message #8: do not install pytest-coverage # This is the commit message #9: pin pytest-coverage # This is the commit message #10: install pinned coverage, do not install pytest-coverage :'( # This is the commit message #11: pin attr # This is the commit message #12: attr is attrs # This is the commit message #13: travis, please tell me attr version # This is the commit message #14: go back to original setup.py # This is the commit message #15: exclude coveralls from 3.4 & 3.5. Try 1 # This is the commit message #16: use [] instead of test # This is the commit message #17: install pytest 5.4.3 # This is the commit message #18: travis tell me something # This is the commit message #19: please do tell # This is the commit message #20: ooohhh? # This is the commit message #21: oh 2 # This is the commit message #22: oh 3 # This is the commit message #23: oh 4 # This is the commit message #24: oh 5

slava added 6 commits November 8, 2017 00:36

update for pypi

2c7c7a6

1) Quaternion.log() returns np.array

1d61426

2) Quaternion.exp() takes np.array

more tests

2d2251c

bump version

cccc1d0

merge master

0bc788a

bump version

4b960ff

slava added 2 commits November 16, 2017 10:02

fixing py3.4 issue: "SyntaxError: can use starred expression only as …

75f583c

…assignment target"

fixing py3.4 issue: "SyntaxError: can use starred expression only as …

0af64dd

…assignment target"

astrojuanlu approved these changes Nov 16, 2017

View reviewed changes

astrojuanlu requested a review from matiasg November 16, 2017 09:56

remove obsolete files

38495b6

slava added 2 commits November 21, 2017 03:04

GeneralQuaternion

a680ae2

fixes

17a258f

matiasg reviewed Nov 21, 2017

View reviewed changes

slava added 5 commits November 22, 2017 09:55

fix sum

a091777

fix error message

be0a40d

fix print

ca5bd83

rename distance to euclidean

a58c4d0

rename Quaternion

16d0b12

slava added 7 commits November 22, 2017 10:23

move average and related methods to Quaternion

27e10e2

zero is not unitary

c2c5bb8

cleanup exp, log

1b5577c

more tests

25c3eee

validation

6d260cc

fix Quaternion.distance

ff0e95e

more tests

15329d0

adapt to py3.4

932d7e9

matiasg approved these changes Nov 24, 2017

View reviewed changes

matiasg merged commit 8f57a95 into master Nov 24, 2017

matiasg deleted the fix_quaternion_comparison branch November 24, 2017 19:48

		@@ -0,0 +1 @@
		Satellogic SA Copyright 2017. All our code is GPLv3 licensed.



		def is_quaternion(q):
		return issubclass(type(q), GeneralQuaternion)

		from quaternions.general_quaternion import GeneralQuaternion, QuaternionError, DEFAULT_TOLERANCE


		ANY_QUATERNION = strategies.lists(elements=strategies.floats(min_value=-5, max_value=5), min_size=4, max_size=4)

Fix quaternion comparison #8

Fix quaternion comparison #8

Conversation

slava-kerner commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

astrojuanlu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrojuanlu commented Nov 16, 2017 via email

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 21, 2017 • edited Loading

matiasg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava-kerner Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

matiasg Nov 21, 2017 • edited by slava-kerner Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017 • edited Loading

coveralls commented Nov 22, 2017 • edited Loading

coveralls commented Nov 22, 2017 • edited Loading

matiasg commented Nov 24, 2017

slava-kerner commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

slava-kerner Nov 21, 2017 •

edited

Loading

matiasg Nov 21, 2017 •

edited by slava-kerner

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading