Nice--I like that this makes use of Table. Per my comment in #351 I think Astropy should also include an offline copy of this table in its installation. So there need to be functions that choose which copy of the table to use depending on which is more recent.

Also, I don't think the file path for the tables (FINALS2000A, etc.) should be configuration items. It should either be found in the Astropy package, or in the user's ~/.astropy/data and probably nowhere else. That would keep things simpler, I think. Though I could see an argument for allowing it to be loaded from an arbitrary path for testing purposes, but that would have to be done explicitly.

@iguananaut - thanks for the comments. I implemented the easy ones, just so this doesn't stall altogether. But I'm a bit unclear about how to proceed, so I thought it would be good to try to clarify what behaviour is expected, following partly your last comments on #351, with questions where I do not really know what to do. Starting from the perspective of Time (assuming just IERS A for now, since this seems to include all relevant information from IERS B):

1. If a UT1 is requested, _get_delta_ut1_utc is called which looks whether _delta_ut1_utc exists -> implemented already
2. If not, needs to get information from the IERS table. This seems to require a fairly complicated chain. First, check whether IERS table has been previously loaded for another UT1 conversion -> how? (also below)
3. If so, skip to 5. If IERS not previously loaded, check whether there is one in ~/astropy/data and whether that is extends further/is newer than package one; if so load -> fairly easy (can one avoid loading both fully? if package is newer, log/warn user that his package version is outdated; just delete it?)
4. If not, load table from astropy package -> easy
5. Check whether the time requested is covered by IERS table -> easy (status in current code)
6. If not, check whether it is well before IERS (1960s) or well into the future, beyond hope for a prediction -> easy
7. If time within reason, download new table and store in ~/astropy/data -> don't know how (though probably not too hard; ask user for permission? log/warn?)
8. retrieve (predicted time) or raise exception that UT1 cannot be calculated -> easy

Here, an implicit assumption is made: we do not want to load the IERS table on startup or on just any invocation of Time, but we also do not want to have to reload the whole ascii table for any new UT1 request. So, should the table stay around for the session? Or be garbage collected with Time? How does one implement this? Somehow attach it to the life Time class copy?

Note that most of this should be in IERS, not in Time, since IERS also provides polar motion (PM_x, PM_y), which is needed for coordinates.

Finally, what if someone insists on using IERS B. Maybe that is a ConfigurationItem? I guess the easier route for that case might be to document the existence of the IERS_B class, and how to use that to get UT1-UTC, which the user then can store in a Time._delta_ut1_utc themselves.

Suggestions most welcome! (same from @taldcroft, @cdeil, @eteq, @astrofrog, @mdboom)

The previous bug I introduced makes clear I should start writing tests. Will do so once there is some consensus on the approach being a good one.

2. If not, needs to get information from the IERS table. This seems to require a fairly complicated chain. First, check whether IERS table has been previously loaded for another UT1 conversion -> how? (also below)

This one is simple enough. Although the IERS_A class interface you've provided is nice, it might make sense to have a function that wraps IERS_A.read() and returns the appropriate table; the result of this function can then be memoized so that future calls to the function can return the already loaded table. That or you could add extra caching options to IERS_A.read(). For example add a use_cached=True option to return an already cached copy of the table (if it exists).

3. If so, skip to 5. If IERS not previously loaded, check whether there is one in ~/astropy/data and whether that is extends further/is newer than package one; if so load -> fairly easy (can one avoid loading both fully? if package is newer, log/warn user that his package version is outdated; just delete it?)
4. If time within reason, download new table and store in ~/astropy/data -> don't know how (though probably not too hard; ask user for permission? log/warn?)

Regarding automatic downloading I'm not sure. I think there should be a ConfigurationItem for this with the default to not automatically download the new table, but to instead just issue a warning that the user's existing table may be out of date, and instructions for how to update the setting to download a new table on the next time conversion. Such a default is good for analysis situations, whereas for scripts and such it could be set to automatically download.

I'm not sure what's best though. The recent tzinfo discussions on Python-dev might be instructive in determining how best to handle this.

This one is simple enough. Although the IERS_A class interface you've provided is nice, it might make sense to have a function that wraps IERS_A.read() and returns the appropriate table; the result of this function can then be memoized so that future calls to the function can return the already loaded table. That or you could add extra caching options to IERS_A.read(). For example add a use_cached=True option to return an already cached copy of the table (if it exists).

Glad to hear it is simple... I really had no idea but as you used a term I did not know (memoize), some googling did the trick. I'll think about class vs function and will try this at home ;-)

Regarding automatic downloading I'm not sure. I think there should be a ConfigurationItem for this with the default to not automatically download the new table, but to instead just issue a warning that the user's existing table may be out of date, and instructions for how to update the setting to download a new table on the next time conversion. Such a default is good for analysis situations, whereas for scripts and such it could be set to automatically download.

I'm not sure what's best though. The recent tzinfo discussions on Python-dev might be instructive in determining how best to handle this.

I'm tending against the automatic download (nicer if results are predictable, independent of the existence of an internet connection). Since it also is easier, I'll just start with that.

One more practical question: what would be the logical place to store the data in the package? A data subdirectory? Or rather a top-level data directory (joining cextern, docs, etc.)

Added comment: how do you get the quoting of someone's message? clearly, ">" from an e-mail did not work... (perhaps I have just lost all options to mark by sending an e-mail. very odd).

@mhvk - this is precisely the sort of situation the utils.data.get_* function's cache option is designed for. You definitely should not put anything in the current directory. Instead, you'd want to do something like this:

from ...utils.data import get_readable_fileobj
with get_readable_fileobj(EOPC04_IAU2000_URL(), cache=True):
    ... do whatever you need to with the file...

That will automatically download the file if it's not present, and if it is, it will use the cached version.

One thing currently missing from this function is a way to specify when a cached file should be refreshed. So far it hasn't been needed, but in this case it's pretty obvious you want to periodically get the bulletins once they're out-of-date. So that leads to a strong reason to implement this behavior (and I added #1162 to address this).

Also, I am strongly against any behavior other than defaulting to downloading the latest bulletin. IERS A comes out weekly, while B is monthly. We certainly don't want to update the source code that frequently, so we have to download it by default if we want to keep it up-to-date. I wouldn't be averse to including one copy in source (which we explicitly don't keep very up-to-date) just so the code works when offline, but then there needs to be a warning along the lines of "couldn't get IERS data. Orientation parameters and DUT1 (UTC-UT1) might be quite wrong." if someone tries to use the source code version if e.g. they are offline.

Also, I am strongly against any behavior other than defaulting to downloading the latest bulletin. IERS A comes out weekly, while B is monthly. We certainly don't want to update the source code that frequently, so we have to download it by default if we want to keep it up-to-date. I wouldn't be averse to including one copy in source (which we explicitly don't keep very up-to-date) just so the code works when offline, but then there needs to be a warning along the lines of "couldn't get IERS data. Orientation parameters and DUT1 (UTC-UT1) might be quite wrong." if someone tries to use the source code version if e.g. they are offline.

My inclination would be effectively lazy-loading the latest table into the cache only at the point when the existing table (either from the source tree or from the cache) does not cover the time span required. I.e. if you are doing transforms in year 2001 then nothing will ever happen, but if you are doing transforms for last week then it will see that the existing table doesn't have those DUT1 values and update the cache file accordingly. This should happen automatically by default. I would have logging output concerning the download at the debug level so for typical users everything is seamless and just works without too much chatter.

I feel pretty strongly that if valid IERS data are not available for a given transformation then this should result in an exception, not a warning. Getting the wrong answer (by a lot or a little) is not an option for Time, and warnings are just too weak since they can easily go unnoticed in automated scripts or by inattentive users (imagine a script that produces a lot of output that scrolls by).

The implication here is that if a user is really offline and needs to do transforms at dates more recent than their cached file, they will have to set dt_utc_ut1 manually. That seems like a reasonable requirement for this corner case. The exception message could even give a hint to that effect.

Here, an implicit assumption is made: we do not want to load the IERS table on startup or on just any invocation of Time, but we also do not want to have to reload the whole ascii table for any new UT1 request. So, should the table stay around for the session? Or be garbage collected with Time? How does one implement this? Somehow attach it to the life Time class copy?

The table should be loaded the first time it is needed and then stay around for the session as a module global. I am never sure about the very best way to do this (maybe @iguananaut) has an opinion, but I usually use one of two options:

1. Define a module global dict, e.g. IERS_DATA, which then gets a key value pair via a load_IERS function (for example).
2. Define a class with an accessor property that lazy-loads the data, then make a global instance of that class to hold the IERS data.

I sort of like 2 better.

1. Define a class with an accessor property that lazy-loads the data, then make a global instance of that class to hold the IERS data.

I realized you already have Time as the logical owner of IERS data so there is no need for a separate container class. E.g.

class Time(object):
    @property
    def iers_data(self):
        if not hasattr(Time, '_iers_data'):
            Time._iers_data = get_iers_data()
        return Time._iers_data

I think something like this should work. The use of a Time class attribute avoids reloading the table for each instance or even for subclasses.

Thanks for the detailed comments:
@eteq - good to hear about get_readable_fileobj. Given @taldcroft's comments about when to update the table, the expiry option may need some thought, though these files just slowly grow, so the download overhead can be fairly small if one uses something akin to wget -c.
@taldcroft - The lazyloading indeed seems reasonable. If someone never uses UT1, or never beyond what's already covered in the copy present, no automatic updates should happen (others: the IERS B data are meant to be final).
@taldcroft - I agree that Time should raise an exception if no IERS value is available. In the implementation I made, Time would do this itself, based on the status the iers routine returns. The cases where only IERS A values or predictions are available may need some special treatment -- probably the user explicitly telling those are OK.
@iguananaut, @taldcroft - Thanks for the tips on how to keep the iers Table alive during one's session. Something akin to what Tom wrote, but in the IERS class, should work (best not in Time, since coordinates will eventually need PM_x and PM_y from the table).

A bit too many commits, and possibly the disappearance of one of my comments, but anyway, it is now possible to do:

from astropy.time import Time
t = Time(['2012-06-30 12:00:00', '2012-06-30 23:59:59', '2012-06-30 23:59:60', '2012-07-01 00:00:00', '2012-07-01 12:00:00'], scale='utc')
t.ut1

which gives

<Time object: scale='ut1' format='iso' vals=['2012-06-30 11:59:59.413' '2012-06-30 23:59:58.413'  '2012-06-30 23:59:59.413' '2012-07-01 00:00:00.413' '2012-07-01 12:00:00.413']>

@taldcroft, @eteq, @iguananaut - I have been thinking a bit about how to proceed with IERS/UT1 and believe it would be better to split off "how to deal with (down)loading the right file" from "how to read IERS and pass UT1-UTC to Time". As what I have nearly completes the latter, I'd suggest to get it working first, assuming a fixed IERS file in some standard astropy location. This would allow us to focus on one part at the time. Does this make sense?

If so, could you check that you think my set-up is OK? A specific question I had was about best practice in memoizing the table: should I just be keeping the columns that are actually used (saving memory)? (As it is, I keep the full table, but it is easy to change.)

For this part, the two questions from the IERS file perspective, are:
(1) What is the proper package location? Simply inside the iers directory? Or in iers.data? Or perhaps in astropy.data?
(2) Shall we keep IERS A or IERS B in the package? Advantage of A is that it contains the most important numbers from B as well as predictions; advantage of B is that it is the definitive version. Since the astropy version will generally not be up to date anyway, my tendency would be to go with B only, and allow A as a user download later. (Note: A and B are both 2.8 MB uncompressed, A is ~800kB gzip'd; B ~600 kB.)

p.s. I did think a little on whether the current structure is OK for the downloading/user-cache stuff, and concluded it was easily done as part of the .open method (e.g., triggering checks and possible downloads by passing JDs already there).

@mhvk - Sure it's fine to "down-scope" this to just parsing for now and separately deal with the download. That makes sense given #1162 is probably needed for that part anyway. I am with @taldcroft that an exception should be raised if the table is "out-of-date" though.

For your two questions:

1. To match convention, you'll want it in astropy/utils/iers/data. But there should not be a package astropy.utils.iers.data, so don't put in a __init__.py file. You'll also need to edit the get_package_data function in astropy/utils/setup_package.py to get it properly included in the installed source. It's pretty obvious how to do this if you just look at the file.
2. I like your suggestion of just including B in-package (gzipped). That also has the advantage that it has a more informative header than A (for those browsing the source code). If we do that, are you not going to implement parsing for A in this PR, then?

(I'll let @taldcroft address the question of how to best store the table - I'm not sure, myself.)

Oh, and @mhvk - it looks like somethings weird with your branch here, and there's an unnecessary merge. The following should do the trick:

(This assumes your remote for the main astropy repo is called "astropy", and yours is called "mhvk" - in practice probably one of them will be called "origin" - which one depends on exactly how you first cloned your local copy)

git fetch astropy
git checkout time-ut1 # you may have done this already
git rebase astropy/master
git push mhvk time-ut1 -f

Even better, you might instead replace the second-to-last step with git rebase astropy/master - that should bring up an editor showing all the commits you're rebasing. You'll see that there are two commits with the message "Cleanup of interfaces". You should either change "pick" to "s" if you want to combine those two commits, or change one of them to "r" if you just want to change the commit message to clarify what's different about them.

I've backed up this branch on my repo (https://github.com/eteq/astropy/tree/mhvk-time-ut1-backup-old), so if you get into any trouble with this, I can help you restore it.

@eteq - OK, I rebased. Also some answers (and questions)
0) As implemented right now, Time raises an exception when any times are outside of the range covered by the table.

1. I've put the data and ReadMe's in astropy/iers/data (see last commit). ----EDITED-- Following was solved in next commit: --EDITED-- I noticed that when I gzip the data, I have to give the name in the ReadMe file a .gz extension too, which is not great, as it then cannot be used on a non-gzipped file any more. Do you know a way to avoid that? Otherwise, I'll raise an issue.
2. I was planning to keep the A parser in this PR. As written, one can pass file and readme to IERS_A and IERS_B, so even now someone could use a downloaded file. (Indeed, I'll document this later.)

@taldcroft, @eteq, @iguananaut - now added documentation within the files as well as further tests (to ensure it doesn't inadvertently break again). I think it is in decent state; if you think so too, I can add further documentation -- I guess this would be both in time and utils (where it will be somewhat an odd-one-out, but as I think it will be needed in coordinates as well, it still seems to make most sense).

@mhvk - awesome. For my part I'm a little swamped now and won't be able to give this big-ish PR a proper look until next week. @iguananaut is on travel as well at the moment, not sure when he returns.

@mhvk - this is looking on the right track to me too - I left a few comments inline, although I didn't do a terribly thorough review of the actual implementation (as I'm not completely up on the time internals).

Oh, and regarding your 1) above: did you manage to correct that? The diff right now seems to have the Readmes as plain text rather than gzipped files...

One final thought: it looks like you're not testing the A parser, quite reasonable given that you wanted to implement the download for that separately. Can you just add a note somewhere prominent in the A source code that indicates it has not yet been tested? Once the downloading is taken care of, we can add a test for it and remove that note, but I wouldn't want someone to try to use it and not realize it's unfinished (or have us forget to add the relevant test, seeing as how B is being tested).

@eteq - I just sent a new commit, which addresses some of the issues you raised:

1. On having self.iers in Time: on further thought, I agree this was a bad choice. Instead, Time should just open the IERS file and assume it is set up correctly. So, I now set it up such that the IERS.open (i.e., on the class itself) by default returns IERS_B. I also added instructions in iers.__doc__ on how one could get it to use something else. Net effect: Time is kept very simple; iers slightly more complicated.
2. On what is exposed: I realised my names were not helpful. I now made more generic ones, which I think make sense to expose for now, so users can more easily find out what is done (they are referred to in the various docstrings).
3. On ConfigurationItems: I had those earlier, but the previous conclusion was that this was not needed, since eventually we'd like most of the updating to be automatic. Maybe OK to keep as is for now?
4. I cannot get MJD_ZERO from time.core since it is not exported ;-) Thinking about it, my sense of trying to ensure packages depend on as few others as possible would argue for keeping it here (indeed, for this reason I decided to remove the dependence on Time, replacing the one isinstance(jd1, Time) with a try-except).
5. Yes, the problem I had with the ReadMe seemingly unable to recognize gzip'd files turned out trivial to resolve (by adding a * to the file name mentioned in the readme); indeed, this makes it more generally useful -- the CDS reader is really great!
6. I actually tested the IERS_A parser myself. Now added that to test_iers as well, with a test that only gets done if IERS_A.open() works (same is done in test_ut1 for Time; essentially, this only works if one has finals2000A.all in one's working directory).

p.s. Am confused with the one failed travis test, which seems entirely unrelated to my PR (2098.10).

@mhvk - responding below to your items

1. It seems a bit overly-complicated to figure out how to switch to IERS A, but that's not really crucial right now - as you say, hopefully we can just make it automatic in the future PR that does the downloading.

2. Yep, it's clearer now, thanks. Although see below re: why some should be configuration items.

3. My interpretation of the discussion above was that you didn't want to be using ConfigurationItems only for files that are downloaded automatically. That is, configuration items should always be used if you ever expect a user to want to do anything with it. If it's a constant in the package that will definitely not be if interest to the user (e.g., a file name or other symbol that is only internally used inside the package), it's fine to just be a regular variable, but anything a user might want to change should be a configuration item. For example, IERS_A_README is fine as a variable because it's just used by iers (although I don't see why it's in __all__ - is it needed somewhere else?), but IERS_A_URL should be a configuration item, because a user might want to use some other URL that has the IERS A bulletin (e.g., some local server that's mirroring the USNO or something). Does that make sense?

Along the same lines, I don't understand why IERS_A_FILE exists. Shouldn't the filename be derivable from either the URL or the file name used in get_pkg_data_filename? Or am I misunderstanding something?

4. Ok, that's fine - I was actually thinking that's why you did it at first, until I saw you had the time import in the file. So I understand the need to duplicate that sort of constant if we're worried about circular imports. (Although this might mean instead time should get MJD_ZERO from astropy.utils.iers? @taldcroft - do you have any opinion on that?)

5. Cool, just wanted to make sure.

6. Ok, that seems fine for now - we'll definitely want to replace that with something that uses the cached version in the follow-on PR. I think it would be better to not look for something in the current directory unless the user specifically asks for it in the final version, but that can wait until we have the downloading stuff working.

Finally, the travis failure was apparently some random glitch - I re-ran the build (something that only the repo owners can do, annoyingly), and it now appears to be passing.

@eteq - some more thoughts (before adjusting too much)

1. I agree the IERS A documentation is complicated. Am also not sure I actually should mention a trick which may not work once we set up the configurable or automatic methods. So, I think it will be best to keep only the description of how to set delta_ut1_utc directly (which is a method that should continue to work even if it is not strictly needed any more).

@taldcroft - I'm hoping to postpone the "just work" part for IERS A, since we agreed this would not be included in the standard astropy package and it thus requires having set up the automatic downloads, etc., which I think is much better done as a separate PR (definitely after I get proper barycentering to work...).

2&3) I think I see what you are saying, but am not completely sure about the conclusion. As it is, users can put there own IERS files as keywords to the .open or .read methods, so in that sense there is not (yet) anything to configure. I exported the keywords mostly so people knew where to find things, and so I could refer to them in the documentation rather than duplicate things and run the risk that they change. If you think it is cleaner, though, I'm happy not to expose them, and adjust the documentation (after all, people can look in the source code!). Perhaps better given that things may still change...

@mhvk - re: 2&3: my interpretation of what you've done here is that we generally don't expect the users to be using iers at all unless they have a special need, right? So keywords to open and read are not so useful, because it forces users to figure out how to go about using iers, instead of just having iers know to use a particular file they've downloaded (or want it to download). So that's why I'm suggesting ConfigurationItems here - a user who just wants Time to work using IERS tables they downloaded (or a URL other than the default) should just be able to set the appropriate configuration item in their astropy.cfg file, and never bother with changing their code at all.

If they want to do something unusual with iers, they can go ahead and use open and read with whatever keywords they want - but for the typical situation of just wanting to have an IERS file somewhere (or a different URL), they should be able to use the configuration system without worrying about iers implementation.

@taldcroft - I rethought the IERS A vs B case, and ended up concluding that it might be better to stick to IERS B by default for now. Partly, it seems better for the default to be "final values or an exception" and partly it is that as long as the automatic download is not implemented, predictions might too easily get used where final values are available. But it is also because I wasn't quite sure how to deal with the configuration item, and thought choices might well change when we do the downloading.

Hence, apart from documentation updates, the main change here is to implement a get_delta_ut1_utc method in Time, following your suggestion to have a conduit to IERS.ut1_utc. As implemented, one can give an alternative IERS table, though that still has to be opened outside of Time (this seemed preferable over giving the ability to give an ascii file, as that would also require being able to tell what type of IERS file it was).

Finally: I do see your concern over the memoization, but am unsure how to address it...

@mhvk - I agree with your approach now and the scope seems good. We can (will) come back to this.

@taldcroft - I think I got them all. Updated the example with IERS_A_URL also in iers.py and time/index.rst.

@mhvk - this looks OK now, you just need to rebase to clear up the merge conflict. Thanks!

@taldcroft - OK, rebased.