Skip to content

Conversation

@cdeil
Copy link

@cdeil cdeil commented Dec 17, 2014

@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:

E               IndexError: (some) times are outside of range covered by IERS table.

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?

@eteq
Copy link
Owner

eteq commented Dec 17, 2014

@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?

@eteq
Copy link
Owner

eteq commented Dec 17, 2014

@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 time is concerned? The assumption that UT1-UTC=0 is of course wrong if leap seconds stop getting added in, but we can cross that bridge when we come to it...?

@mhvk
Copy link

mhvk commented Dec 17, 2014

@eteq - for Time, we discussed this at some length and ended up deciding that by default the code should be strict, partially because often it will just be that the iers table needs to be updated. Right now, the not-so-easy way avoid the exception it is to explicitly set time.delta_ut1_utc = 0 before trying to convert to UT1.

I still think the decision to raise by default is right, but would hope we can make it easier to just ignore the iers correction altogether by some explicit keyword or so (say, iers=False or full_accuracy=False). Note that the iers package does allow one to avoid the exception, by calling the methods with return_status=True, which leaves it up to the caller to decide what to do with the points which are outside the iers range.

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 Time, one of the test cases @taldcroft used just went over a list in the erfa documentation. Is there something similar for coordinates?

@eteq
Copy link
Owner

eteq commented Dec 17, 2014

@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 Time, usually you really care a lot about that fine level of precision.

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 Time, instead have it fail with an error?

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)

@cdeil
Copy link
Author

cdeil commented Dec 17, 2014

+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.

@mhvk
Copy link

mhvk commented Dec 17, 2014

@eteq - agreed that the cases are different: yes, I think it is fine to default to a warning inside SkyCoord. Do you use time.ut1 in lots of places, or could you just wrap a single instance in a try/except?

As for IERS B vs A: I think shipping IERS B with astropy is the right call, as we don't want people relying on predictions when the real data are available. But it would be wonderful to have the IERS file automatically updated, in which case IERS A would be fine (as it has the B data anyway). But this needs some clever download skills that I don't have -- see astropy#1697.

@eteq
Copy link
Owner

eteq commented Dec 17, 2014

@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.

@mhvk
Copy link

mhvk commented Dec 17, 2014

@eteq - sounds good. I would definitely postpone the downloading to 1.1! I think the user-facing side of that needs some thought...

@cdeil cdeil force-pushed the coords-icrs2altaz-tests branch from 991d53b to d08cb21 Compare December 17, 2014 21:00
@cdeil cdeil force-pushed the coords-icrs2altaz-tests branch from d08cb21 to a68db7c Compare December 17, 2014 21:06
@cdeil
Copy link
Author

cdeil commented Dec 17, 2014

I've attached a few more tests against results from other packages.
None of them give comparable results with Astropy at the moment ... probably I'm making stupid mistakes ... I'll try again tomorrow.

@cdeil cdeil changed the title Add hor2eq coordinate conversion tests WIP: Add AltAz to RaDec conversion tests against other packages Dec 17, 2014
@eteq eteq merged commit a68db7c into eteq:coords-icrs2altaz Dec 19, 2014
@cdeil
Copy link
Author

cdeil commented Dec 19, 2014

@eteq - Sorry I didn't have time to finish this today ... I'm surprised you merged this in this state.
Anyways ... I'm online now and can do some more testing ... let me know if there's something specific where I could be useful.

@eteq
Copy link
Owner

eteq commented Dec 19, 2014

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants