Vectorize

[jeanconn]

Redo vectorized approach.

[jeanconn]

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]

Needs updated tests.

[jeanconn]

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

[jeanconn]

Does this also need rebase?

[javierggt]

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

[taldcroft]

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]

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]

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]

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]

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.

[javierggt]

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]

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.

[javierggt]

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.

[taldcroft]

For performance in the scalar case, I think the only really critical case is annie. So one could just do a 10 ksec sim with this branch and with release and see if it matters.

[taldcroft]

@jeanconn - could you take a quick look at annie performance?

[jeanconn]

Regarding annie performance, sure happy to take an independent look, though I note the "classic use case" @javierggt has also calls an annie sim (my take-away being he is set up to do that annie benching too if I'm not fast enough for this thread).

[taldcroft]

@jeanconn - ah, then nevermind, @javierggt can just do it.

[javierggt]

so... stupid question...

I got the code that uses annie. I suppose that what you would like to time is this part:

sc = Spacecraft()
sc.track_stars_setup(**annie_kwargs)
sc.run()

Right?

[jeanconn]

That makes sense to me. For that somewhat "canonical" example, the loop I had at the bottom that calculates delta quaternions

est_delta_pitchs = []
est_delta_yaws = []
times = []
for att_record in sc.telem.att_records:
    if att_record.att_est is not None:
        # For arbitrary lists of quats, we'll often just have
        # the 4 vector, even if the annie example already
        # has Quat objects for est, true, and cmd.  Here, I've just used the
        # ".q" of these Quat objects to reinitialize them.  I don't know if
        # it makes any difference for your testing
        dq = Quat(att_record.att_est.q).dq(att_record.att_true.q)
        est_delta_pitchs.append(dq.pitch * 3600)
        est_delta_yaws.append(dq.yaw * 3600)
        times.append(att_record.time)

is the kind of thing we do with the annie data, but in previous use that happened outside the simulation and as such the loop doesn't need to be timed; especially if we'd have a new recommended new way of doing it with the new code.

For benching the annie sims, there might be value in timing the 1000s simulation I had in that example and timing/benching a longer one (20ks?) just to check that nothing is weird with how it scales.

[javierggt]

right, but that loop I am already benchmarking it separately. I ended up splitting the statement

dq = Quat(att_record.att_est.q).dq(att_record.att_true.q)

into

        q = Quat(quaternions_true[i])
        dq = q.dq(quaternions_est[i])

so one can see each step

[jeanconn]

Yep. That's what I meant by that loop doesn't need to be timed and we'd have a new way of doing it :+1:

[javierggt]

What way would be best to share a notebook? I can put it in google drive.

The bottomline is sc.run() is the same for all versions. There might be differences in the loop that comes afterwards

Each method (non-vectorized, running over quaternions one by one) timed: https://drive.google.com/open?id=1IS5KJ1iwB_cTkJZfa5y92GoF03g75OwN

Annie timed: https://drive.google.com/open?id=13_6ifBY4DPRaOyfYyFQ-bSRzv3kDCElR

[jeanconn]

For notebooks we don't have a one-size-fits-all when it comes to sharing. If you think they will have value the next time we work in Quaternion, I think they could live in the repo. If they are one-offs, google drive is fine though if they can be public, often we'll copy to a web area and serve with the web server (and then share links via nbviewer). Sometimes we'll check in a notebook into the separate skanb project, though that method has not taken off.

[javierggt]

ok, I made most changes.

What is left is:

• what @taldcroft mentioned above about validation of vectorized operations.
• something I do not understand yet (it is commented in the test). Most probably this results from conventions on how to handle degeneracies. These conventions should be tested.

[javierggt]

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

[taldcroft]

Sadly, developer is not allowed to review. :smile: 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]

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

[taldcroft]

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

[javierggt]

I'm ok with merging

[taldcroft]

Woohoo! Now on to the docs PR?