Repository navigation
add SkyCoord.position_angle and associated test - #2487
Conversation
There was a problem hiding this comment.
Does assert = work for this case? I would think assert_allclose would be better to compare floats?
There was a problem hiding this comment.
unfortunately, assert_allclose is a bit awkward to use because it compares the values directly, which is wrong here because the resulting PA is in radians, not degrees. Of course there's ways around this, but they look a bit more awkward... And this passed fine for me locally. If the tests pass on all python versions, I think it's probably safe here, but if not I'll change it to use assert_allclose with the appropriate .to calls
There was a problem hiding this comment.
This seems to work on travis-ci and also passes on my Mac.
But assert == for floats is still a gamble and it's unlikely that this passes for others that have e.g. a 32 bit machine or an Intel compiler (they trade some precision at the EPS level in trigonometric functions in favour of speed) or ... I'd bet that I can make this fail somewhere if I try a few machines / compilers. :-)
|
Thanks for implementing this, @eteq . |
|
Alright, the commit I just pushed up I think should address all of @cdeil's comments. |
|
Yes, all my comments have been addressed. I'd say this is ready for final review by someone else and a simple addition, so could still go in 0.4. |
|
Alright, I tentatively labeled it 0.4, but I'll leave it to @astrofrog to make the final call. |
|
Looks good to me! |
add SkyCoord.position_angle and associated test
This closes #2311 by adding a
position_anglemethod toSkyCoord.This is pretty straightforward, and #2311 was delayed to await #2422 , but this is still a new feature. So it depends on how strict we want to consider "feature freeze" for deciding if this is 0.4 vs 1.0 . Of course I'm biased as the author, so I'll leave it to @astrofrog to decide.
cc @richardgmcmahon @cdeil