sot / Quaternion

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

Add delta quaternion method and other package modernization #2

Closed taldcroft closed 8 years ago

taldcroft commented 8 years ago

This brings Quaternion up to standards for package structure and adds some stuff:

taldcroft commented 8 years ago

@jeanconn - because of the PEP8 update it may be best to look at individual commits or the diffs from the PEP8 commit through to the end.

taldcroft commented 8 years ago

Anyway, I think this is basically ready to go while the vectorizing will take some time. The one thing I forgot to do is put in a repr.

taldcroft commented 8 years ago

Ready for final review.

jeanconn commented 8 years ago

The only other thing I notice is that 180 used to be an acceptable value for _ra0. Hopefully there's nothing out there using this private value anyway.

taldcroft commented 8 years ago

I think that -180 <= val < 180 is the more common way to do this, so as you say hopefully nothing was using the private value and depends critically on that distinction.

jeanconn commented 8 years ago

Just for kicks, I let github do that search

https://github.com/search?q=org%3Asot+ra0&type=Code

Looks like nothing is using it, and if we want we could update Ska.astro and the Perl Quat at some point to have the same convention.

jeanconn commented 8 years ago

Also :+1:

taldcroft commented 8 years ago

Ska.astro should be deprecated (replace with astropy functionality) and removed. There are a few real dependencies. But certainly not a high priority. :-1: on touching any legacy perl code.

jeanconn commented 8 years ago

Well yes, when I say "if we want" I realize that the answer is already no on the Perl code.

taldcroft commented 8 years ago

This now fixes #1. (I made a mistake in the commit comment). The suggested test does not fail for me (assuming the angle += 0 was supposed to be angle += 1), so I did not include a test.

jeanconn commented 8 years ago

Sorry, the test was supposed to be

angle += 0.1

On Fri, Jul 15, 2016 at 10:51 AM, Tom Aldcroft notifications@github.com wrote:

This now fixes #1 https://github.com/sot/Quaternion/issues/1. (I made a mistake in the commit comment). The suggested test does not fail for me (assuming the angle += 0 was supposed to be angle += 1), so I did not include a test.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sot/Quaternion/pull/2#issuecomment-232973073, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwT1IXA4Kz4CvbSrFwmk8o700wbO8Aoks5qV551gaJpZM4JLsCy .

jeanconn commented 8 years ago

I stuck a version of the test (with my typo fixed) in the tests. Acceptable?

taldcroft commented 8 years ago

Looks good. I just pushed a tweak to make the test name more relevant and add a descriptive docstring.

taldcroft commented 8 years ago

Interestingly, if I change the test to loop over np.arange(0, 360, 0.1) then it never generates the error. Obviously it's a fiddly numeric issue.

taldcroft commented 8 years ago

@jeanconn - thanks for review and inputs!