Vectorize #5

jeanconn · 2016-11-18T02:30:10Z

Redo vectorized approach.

Quaternion/Quaternion.py

jeanconn · 2016-12-08T13:26:43Z

For a bunch of current work, it would probably be helpful to have the one, vectorized Quaternion approach instead of a mix of current Quaternion and mica.quaternion whenever vectors are needed. I'm not sure if there are any showstoppers in this implementation.

jeanconn · 2016-12-08T14:21:09Z

Needs updated tests.

jeanconn · 2019-09-04T14:46:06Z

Move to explicit keywords for the input types (but still supporting the old API for single values supplied as arguments).

jeanconn · 2019-09-07T01:31:05Z

Does this also need rebase?

javierggt · 2019-09-07T01:41:15Z

or merge. What I did in my local copy for now is to merge into master. Otherwise the test does not work.

taldcroft · 2019-09-07T01:51:35Z

Our workflow (inherited from astropy) is always rebase to get rid of conflicts, never merge master. I'm not precisely sure of the reason, but in astropy anyway whenever contributors accidentally do a merge it tends to be a disaster and they get like 50 unrelated commits showing up in their PR.

javierggt · 2019-09-07T01:58:16Z

but one should not rebase a branch that was already pushed. I have done merges to branches without problem.

In any case, I did the merge after pushing, so it should not be in github.

Right now gitk kills my OSX session, so I can't inspect the index before pushing, which I normally do. Hopefully I did not break anything.

javierggt · 2019-09-07T02:01:50Z

by the way, I understand about not merging master, I am not advocating it. I suppose cherry-picking from master is the right way. If we rebase on a branch that was pushed already, shouldn't one then create a whole new branch?

jeanconn · 2019-09-07T02:03:28Z

Given that is is just us with this branch and we're likely to have one developer at a time working in the branch, I'm good with force pushing the branch after rebase.

taldcroft · 2019-09-07T02:11:04Z

Agreed with @jeanconn. Even in the wild world of astropy we rebase and force push branches all the time. Reviewers can just delete their local version and re-fetch as needed.

The only hard rule is never force push to master or a release branch.

…sue #9

javierggt · 2019-09-07T02:18:53Z

ok. Force-pushed.

When making the changes for arbitrary shapes, there was some juggling with axes, because the transpose function does not do what we want. I used np.moveaxis. I think I can go over the code once again to see if that can be written better.

taldcroft · 2019-09-07T02:35:51Z

Yeah, the axis juggling was why I had thought about numba + simple loops on the outside. But if you are able to get it right with axes then that's great.

taldcroft

Mostly trivial except the _is_scalar issue. I just got 2/3 way through but have to quit for now.

Quaternion/Quaternion.py

Quaternion/tests/test_all.py

javierggt · 2019-09-09T22:44:43Z

I added a _shape data member (instead of _is_scalar). I figure it is more explicit. Then I replaced the if+squeeze statements by a reshape operation. If anyone has strong feelings about it, I can introduce the squeeze statements instead.

Quaternion/Quaternion.py

… modified by accident

…mplementation

javierggt · 2019-09-17T16:43:01Z

I do not intend to make more changes in this branch. All remaining issues are reported in #10, #11, #12, #13 and #14.

taldcroft · 2019-09-17T17:42:15Z

Quaternion/tests/test_all.py

+    t = [[9.25416578e-01,  -3.18795778e-01,  -2.04874129e-01],
+         [1.63175911e-01,   8.23172945e-01,  -5.43838142e-01],
+         [3.42020143e-01,   4.69846310e-01,   8.13797681e-01]]
+    _ = Quat(t)


What is this testing?

No idea. I am removing it. A few lines above there is a test for this interface.

taldcroft · 2019-09-17T18:10:18Z

Quaternion/tests/test_all.py

+    q2 = Quat(q=q1.q)
+
+
+def test_from_eq_vectorized():


The unrolled tests below are excellent. Can you also add the tests like below, that are the most simple way for my brain to be sure that everything works as expected. You might need to remove the dec=90 values from the input, not sure. These are at least partially redundant, but not entirely (I think).

atol = 1e-12 # Hopefully this works? # Compare vectorized computations for all possible input/output combos # with the same for the scalar calculation. q = Quat(equatorial=equatorial23) assert np.all(q.q == q23) assert np.all(q.equatorial == equatorial23) assert np.all(q.transform == transform23) q = Quat(q=q23) assert np.all(q.q == q23) assert np.allclose(q.equatorial, equatorial23, rtol=0, atol=atol) assert np.allclose(q.transform, transform23, rtol=0, atol=atol) q = Quat(transform=transform23) assert np.allclose(q.q, q23, rtol=0, atol=atol) assert np.allclose(q.equatorial, equatorial23, rtol=0, atol=atol) assert np.all(q.transform == transform23)

This test fails because of the points I chose, which are gimbal locks:

assert np.allclose(q.equatorial, equatorial_23)

Then I changed the points to some pseudo-random values and it also fails because q.equatorial can return, for example, roll=-30 or roll=330 depending on I don't know what. Angles are not supposed to be normalized? If not, I can check that they are the same module 360 or something like that.

and this one fails because some rows in q_23 have negative scalar component, which gets flipped in setter:

assert np.all(q.q == q23)

so... I added a helper function to deal with roll values out of the (0, 360) interval. Perhaps this should be fixed in the class.

I am also checking that q gets flipped when needed.

I added this test and it is passing. Look OK @javierggt ?

reply to this is below.

taldcroft · 2019-09-17T18:13:23Z

Quaternion/tests/test_all.py

+
+def test_normalize():
+    a = [[[1., 0., 0., 1.]]]
+    _ = Quat(q=normalize(a))


Need to check the result.

Quaternion/tests/test_all.py

taldcroft

Mostly there, looks great! I added a few more comments and there were a couple of leftover ones.

- removed gimbal lock values from test quaternions (these might fail for now) - added normalize_angles function to deal with the fact that roll does not appear to be normalized - testing all possibilities of conversions - minor fixes

… code swaped the axes TWICE, once using transform.swapaxes and one with a minus sign in all antisymetric terms

javierggt

Well, I decided to approve it :)

taldcroft · 2019-09-20T10:54:31Z

Sadly, developer is not allowed to review. 😄 I think there is a way to enforce this in GitHub, but we just do this procedurally. You can instead apply the "Ready for final review" label when you think it is finally done.

taldcroft · 2019-09-20T10:55:31Z

Will review on Monday (trying to make time for "science" today).

Tests updated accordingly. Also improved error reporting in __init__ and make sure argument can be an array of float, not just an array.

javierggt · 2019-09-26T14:04:20Z

Quaternion/tests/test_all.py

+    assert np.allclose(q.equatorial, eq_23, rtol=0, atol=atol)
+    assert np.all(q.transform == transform_23)
+
+


all these tests were already there:

The first three assert are in line 131 and on

the next three (including the angle normalization) are in line 224 and on

the last three are in line 195 and on

Having them together means the angle normalization is done only once.

While checking them, I realized that these values now do not check for the q flipping any more.

Ah I see, excellent. But it doesn't hurt to have redundant tests, and these are organized in a way that works for my brain. As long as you agree these tests are valid then I'd like to keep them in.

I'm ok with that.

taldcroft · 2019-09-26T14:35:15Z

@javierggt - is the flipping thing something you can address in a post-merge fix? From my perspective this looks good to merge now.

javierggt · 2019-09-26T14:59:27Z

I'm ok with merging

taldcroft · 2019-09-26T15:12:53Z

Woohoo! Now on to the docs PR?

jeanconn commented Dec 8, 2016

View reviewed changes

Quaternion/Quaternion.py Outdated Show resolved Hide resolved

Quaternion/Quaternion.py Outdated Show resolved Hide resolved

Quaternion/Quaternion.py Outdated Show resolved Hide resolved

Quaternion/Quaternion.py Outdated Show resolved Hide resolved

jeanconn and others added 9 commits September 6, 2019 22:12

Vectorize

a267d3c

Tweak quat2transform docstring

0d0b2e9

Tweak variable names and operation order in quat2transform

67cd2dc

Add Tom as first author

9f18663

Fix typo in dimension checks

4a3fb4b

Comment out repr now as not sure what to do with it

c3de472

uncommented and fixed __repr__

8a0f78c

generalized for arbitrary shape and updated interface according to is…

f7c0cab

…sue #9

removed function that should not have been there

c54f4a2

javierggt force-pushed the vector branch from 5b7541c to c54f4a2 Compare September 7, 2019 02:13

javierggt changed the title ~~Vectorize~~ WIP: Vectorize Sep 7, 2019

taldcroft reviewed Sep 9, 2019

View reviewed changes

changes after comments from Tom

be806f2

taldcroft reviewed Sep 9, 2019

View reviewed changes

Quaternion/tests/test_all.py Outdated Show resolved Hide resolved

added self._shape

fa3afab

added comment explaining moveaxis

ba3052f

javierggt added 2 commits September 11, 2019 09:28

author list formatting

60b3167

made shape attribute public and using it in tests

40888bc

taldcroft reviewed Sep 11, 2019

View reviewed changes

Quaternion/Quaternion.py Outdated Show resolved Hide resolved

javierggt added 3 commits September 11, 2019 10:18

make shape property read-only

7068388

make sure data members are copies of given arguments, so they are not…

aa047a1

… modified by accident

removed hardwired numbers and setting them using the non-vectorized i…

48c466d

…mplementation

taldcroft reviewed Sep 17, 2019

View reviewed changes

javierggt added 2 commits September 19, 2019 09:28

More test fixes:

687d26b

- removed gimbal lock values from test quaternions (these might fail for now) - added normalize_angles function to deal with the fact that roll does not appear to be normalized - testing all possibilities of conversions - minor fixes

removed comment about copy/pasted code with swaped axes, because this…

fdbe74f

… code swaped the axes TWICE, once using transform.swapaxes and one with a minus sign in all antisymetric terms

javierggt approved these changes Sep 19, 2019

View reviewed changes

taldcroft added enhancement Ready for Final Review labels Sep 20, 2019

javierggt and others added 2 commits September 20, 2019 11:51

Changed dq method so it takes the same input as __init__.

6f273cb

Tests updated accordingly. Also improved error reporting in __init__ and make sure argument can be an array of float, not just an array.

Add test of vector to scalar correspondence

c2c7bfc

javierggt reviewed Sep 26, 2019

View reviewed changes

taldcroft approved these changes Sep 26, 2019

View reviewed changes

taldcroft merged commit 5b2053f into master Sep 26, 2019

taldcroft deleted the vector branch September 26, 2019 15:12

javierggt mentioned this pull request Sep 30, 2019

Define new API for Quat #9

Closed

jeanconn mentioned this pull request Oct 11, 2019

Vectorize #3

Closed

jeanconn mentioned this pull request Jan 16, 2020

Release 2020.1 sot/skare3#254

Merged

22 tasks

jeanconn mentioned this pull request Feb 13, 2020

Release 2020.2 sot/skare3#299

Merged

javierggt mentioned this pull request Apr 16, 2020

ska3-matlab update for 2020_005 sot/skare3#297

Merged

		assert np.allclose(q.equatorial, eq_23, rtol=0, atol=atol)
		assert np.all(q.transform == transform_23)

Vectorize #5

Vectorize #5

Conversation

jeanconn commented Nov 18, 2016

jeanconn commented Dec 8, 2016

jeanconn commented Dec 8, 2016

jeanconn commented Sep 4, 2019 • edited Loading

jeanconn commented Sep 7, 2019

javierggt commented Sep 7, 2019

taldcroft commented Sep 7, 2019

javierggt commented Sep 7, 2019

javierggt commented Sep 7, 2019

jeanconn commented Sep 7, 2019

taldcroft commented Sep 7, 2019

javierggt commented Sep 7, 2019

taldcroft commented Sep 7, 2019

taldcroft left a comment

Choose a reason for hiding this comment

javierggt commented Sep 9, 2019

javierggt commented Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierggt Sep 18, 2019 • edited Loading

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

taldcroft left a comment

Choose a reason for hiding this comment

javierggt left a comment

Choose a reason for hiding this comment

taldcroft commented Sep 20, 2019

taldcroft commented Sep 20, 2019

javierggt Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft commented Sep 26, 2019

javierggt commented Sep 26, 2019

taldcroft commented Sep 26, 2019

jeanconn commented Sep 4, 2019 •

edited

Loading

javierggt commented Sep 17, 2019 •

edited

Loading

javierggt Sep 18, 2019 •

edited

Loading

javierggt Sep 26, 2019 •

edited

Loading