-
Notifications
You must be signed in to change notification settings - Fork 0
WIP: Add AltAz to RaDec conversion tests against other packages #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This is a good resource of test cases ... it's BSD licensed, so OK to adapt test cases from them: |
|
@cdeil - that's because the IERS tables (necessary both for UT1-UTC and for the polar motion) are only good until ~2015. My guess is that IDL and XEPHEM just don't do these corrections - Polar motions are mostly unpredictable and have to be looked up with IERS, and UT1-UTC depends on when leap seconds are introduced. But they're pretty small effects (well less than an arcsec), so they're often neglected. Probably we should just show a warning but make some guesses (e.g. UT1-UTC ad PMs =0) when in the future instead of failing? |
|
@mhvk and @taldcroft - what do you think about my proposal above for when we're off the end of the IERS tables as far as |
|
@eteq - for I still think the decision to raise by default is right, but would hope we can make it easier to just ignore the All this said, for the present PR, I'm not sure it isn't better to ignore test cases that are out of the iers range, since the precision will be lousy anyway. p.s. For |
|
@mhvk - hmm, I think the problem here is that we're at cross purposes. For coordinates, these are both small effects that 95% of users don't care about. But they do want to be able to do ICRS->AltAz a year or so in the future. (observing planning!) So I'm strongly against erroring-by-default here. In contrast, for Maybe the thing to do is to have IERS used inside the coordinate packages issue a warning but just do DUT=0 and PMs= 0ish? But for regular "user" usage of Also, @mhvk, is there a strong reason for the current situation of defaulting to IERS B and not A? A provides predictions ~1yr in the future, which would help the situation a lot. (For example, right now, you cannot ask what the ICRS->AltAz conversion is right now: it throws the IERS error) |
|
+1 to making it easy to use less precise transformations ... I think a one-time warning that can be suppressed with an option to suppress it would be easiest ... if that is acceptable to those here concerned with safety and users like me not knowing what they are doing. |
|
@eteq - agreed that the cases are different: yes, I think it is fine to default to a warning inside As for IERS B vs A: I think shipping IERS B with |
|
@mhvk - gotcha. I'll update this to instead warn when out-of-bounds. We'll see if I can get to auto-downloading of IERS A, then... I know how to do that but may not have time. |
|
@eteq - sounds good. I would definitely postpone the downloading to 1.1! I think the user-facing side of that needs some thought... |
991d53b to
d08cb21
Compare
d08cb21 to
a68db7c
Compare
|
I've attached a few more tests against results from other packages. |
|
@eteq - Sorry I didn't have time to finish this today ... I'm surprised you merged this in this state. |
|
@cdeil - I merged it because I'm doing other work on the branch that would have made it awkward to rebase this. But feel free to make some modifications to these tests in a separate PR. I'm trying to get through everything else on the original PR in the next few hours. |
@eteq Here's a first attempt at a "verified" AltAz to RaDec conversion tests.
It doesn't work currently because the time chosen in that example is from 2041 ... I get this error:
Apparently IDL and XEPHEM could compute this transformation ... what do I have to do to convince Astropy to do it too?
Do you have any other test cases from books, papers or tests in other software packages that we could put in here?