sot / Quaternion

Quaternion manipulation
https://sot.github.io/Quaternion
BSD 3-Clause "New" or "Revised" License
3 stars 7 forks source link

Make Quat behave more like ndarray (item access, iteration, reshaping etc) #37

Closed taldcroft closed 3 years ago

taldcroft commented 3 years ago

Description

This uses the astropy ShapedLikeNDArray class to provide support for a number of ndarray methods and in particular __getitem__ and iteration. (ebbde49)

In addition:

Testing

For a quick demo: https://gist.github.com/taldcroft/a70fe6c206a9d6203fda1d5aaea9855e

@mhvk - check it out, NICE!

taldcroft commented 3 years ago

@mhvk @javierggt - one hiccup I ran into is that in this case the "internal data item" is a 4-vector (Quaternion), so there is this trickery that the shape of the Quat is self.q.shape[:-1]. The problem is that the reshaping commands need to apply to the shape dimensions except for the last.

I worked around this by applying methods to each of the four components individually and stacking them back together. This seems to work except that a copy is always made. Since this is really not good for __getitem__, I had to write that method separately, and others like ravel will end up making copies in a not-intuitive way.

It doesn't matter all that much, but I'm wondering if there is any way to finesse this. I'm thinking of EarthLocation that has an "item" as a structured array with x, y, and z. Is there some numpy analog of that where the "item" is a 4-vector? I thought that q = np.zeros(3, dtype='4f') looked promising, but then q.ravel() gives shape=(12,) not the desired (3,).

mhvk commented 3 years ago

@taldcroft - the trick I used in astropy.uncertainties is to make the items a structured array. That way the shape ignores the structure, which in this case would have 4 elements. If you want the 4 elements to have different names, you could use something like np.dtype([('a', float), ('b', float), ('c', float), ('d', float)]), but otherwise just something like np.dtype([('quat', '4f8')])

taldcroft commented 3 years ago

@mhvk - bingo! See 095aafc. I didn't change the global data structure, but just did a little structured-array sandwich around apply_method to make it work.

BTW, I left __getitem__ as a method override instead of delegating to apply_method. Doing this gives a factor of 8 better performance.

mhvk commented 3 years ago

Happy to hear that it worked well! And definitely makes sense to override ShapredLikeNDArray defaults that use _apply if you can get a nice speedup.

taldcroft commented 3 years ago

This is ready for review.

taldcroft commented 3 years ago

@javierggt - comments addressed.

taldcroft commented 3 years ago

Thanks for the review @javierggt, very helpful. This needed a rebase which I just did, so merging.

taldcroft commented 3 years ago

One thing I realized is that if we push this to PyPI for others, the new dependence on astropy could be a little annoying. I think I'm just going to pull in that shapes.py from astropy to remove the big astropy dep.

taldcroft commented 3 years ago

So stand by for a new PR here and a 4.0.1 release.