Thought I might as well continue anyway, now including all simpler models that ERFA/SOFA provide, and also allowing the calculation of local time.

For the latter, I had to change the internal use of lon, lat: I made default None, so that exceptions can be raised if a local sidereal time is requested. While doing that, I also swapped the order in the call sequence to match convention (and coordinates); I inserted a test that checks that lon, lat are not given as positional parameters (which should be exceedingly rare). Furthermore, I changed the internal representation to Longitude, Latitude so that one can pass on all the angle goodies (these classes will only loaded if lon, lat are defined).

One issue I have is that I do not know how to test this. I found one on-line SOFA application [1], but it seems not functional. Any suggestions?

[1] http://www.erdrotation.de/ERIS/EN/Eris/InteractiveTools/Sofa/sofa.html

The gmst and similar stuff looks good to me, but I do not think we should put local sidereal times in Time objects. Instead, there should be a location object (probably in coordinates) that has an LST method that you feed a time. Conceptually, I think it makes more sense to ask for the local sidereal time at a location (loc.lst(timeobj)) rather than the local sidereal time of a time at a location (timeobj.lst(loc)). See mhvk#5 for what I have in mind here.

@eteq - first some answers to two small comments -- keeping them out-of-code since they are not specific to the current commits:

• I'd prefer to keep gmst over greenwich_mean_sidereal_time, which seems overly long, and somewhat contrary to the abbreviations used for the different time scales (maybe @taldcroft can comment).
• As is, the precession(-nutation) model keys are included in the various doc-strings; should I export them another way? (beyond actually updating doc/time -- I've been holding off on that until I've had more feedback).

For whether to move lst to a new earth-location class: I agree that often when one wants sidereal time, one will be considering an observatory, and would like it for a given time, i.e., would like to do, say, antu.lst(some-time). But the converse is also possible. E.g., for a project on a binary pulsar, I have sets of UTC arrival times with observatory codes, and sets of radial velocities at given times, again from a range of observatories. For both these cases, I find it conceptually much easier to have Time arrays -- as the location is really secondary (and maybe we can allow location arrays inside -- works for sidereal time already, but not yet for tcb/tdb). Hence, I think there is a place for time(..., loc).lst().

Note that the location is given in the initialiser: for full precision, Time needs to be given the location anyway, since otherwise it cannot accurately transform to the barycentric timescales tdb and tcb. Given that it already has (the option to have) this information, it seems a bit strange not to provide the simple addition lst=gmst+longitude.

All this being said, we do need an earth-location class! But time and location are interdependent, as neither is properly defined without the other (at least to VLBI accuracies, where continental drift matters). My suggestion for now would be to simply enable getting local sidereal time through both. Perhaps you can make a PR for what you had to astropy itself rather than to this PR?

On the new class: While I think basing it on SphericalCoordinateBase is good, it will need some way to initialize from X,Y,Z coordinates, as these are the standard ones in use these days -- longitude, latitude, and elevation are derived parameters. The corresponding routines are all in erfa (e.g., eraGc2gd) -- another reason for all of erfa to be vectorized (and moved to utils) -- you should not need to define, e.g., the WGS84 ellipsoid in your code.

@mhvk - first on the two small items

• Remember that one of our guidelines is for all code to be readable and spell out words when possible. gmst just isn't familiar enough for most people. Perhaps a better solution is to use greenwich_sidereal_time or even g_sidereal_time (I prefer the former, but the latter is at least clearly a sidereal time), and add a mean argument or something like that? It's fine to move our user-facing interface far from ERFA. In fact, that's desirable, as it's very non-pythonic!
• Ah, I missed that it was in the docstrings - but yes, I was mainly thinking about the narrative docs, as references/explanations that a typical user can understand would be valuable. But it's fine if you just haven't gotten to it yet.

This leads to another question, though: should the precession stuff really be in time? It might be more natural to put it in coordinates. Or better yet, we may want to move everything into astropy.utils.erfa or something like that, as some of it is certainly going to be shared (and circular imports might end up an issue). I think none of that is user-facing right now, so we can decide that for the next version, but something to keep in mind.

As for where sidereal_time should live: I agree with you that sometimes it's more convinent to think of the LST at a place for a given time, and sometimes it's at a time for a given place. So I agree there's a place for both, in principal. My main problem here is with using the lat and lon stored in Time. We definitely do not want to have to replicate everything in coordinates in Time. I can see huge amounts of confusion stemming from situations like

t = Time(..., lat=a, lon=b)
c = EarthLocation(lat=c, lon=d)
local_sidereal_time(c, t)

which is the right lat/lon to use? The EarthLocation's, or the Time one? It's intrinsically ambiguous. c.sidereal_time(t) is only slightly better, but it at least shows which is the dominant one. Honestly, the better solution is to figure out how to get rid of lat and lon in Time, I think. But then we have to determine how best to get the lat/lon information to Time in the places it's needed for conversion. Maybe @taldcroft has thoughts here.

On the new class: that was sort of a base-line idea, actually, not necessarily complete. It will all make much more sense once the IAU2000 ICRS <-> ITRS transformation are implemented (next on the Coordinates TODO list after the arrayifying/Quantity-compatibility that's currently happening). Then we can use the same representation for EarthLocation, and esentially all the conversions come out for free. (Also, FYI, it can initialize based on x,y,z - that's built into SphericalCoordinateBase it's just wrong right now because it assumes spheres, not ellipses, which is pretty easily fixable once the above stuff is worked out.)

So how about this as a temporary middle-ground: leave greenwich_sidereal_time/gmst as Time method, but then make sidereal_time a function in astropy.time with a signature either of or sidereal_time(time,(lat,lon)). It will be natural to just bring in the new coordinate class once it makes sense to do so, then, as the second argument. We could also consider adding methods to both Time and EarthLocation (or whatever) at that time.

@mhvk - I have a suggestion for you to consider here: perhaps you should de-scope this PR to only include greenwich sidereal time. That shouldn't be particular controversial and we could merge it pretty quickly within the 0.3 timeline (e.g., by the end of the coming week). Then we can visit LST for the next version, but in the meantime anyone who wants to can at least access GST.

If you're willing to do this, I think there's only two major items:

• gmst and gast are too short/jargony for Astropy function/method names. I would advocate a single greenwich_sidereal_time method, with an apparent keyword that can be True or False
• Consider adding a bit more in the narrative docs about the different options for precession. If there's not time to do that, I'm still fine merging this without more than what's in here now, but it would be nice if possible.

@eteq - I would like to have @taldcroft have a look first. I don't mind if it is beyond 0.3

@mhvk - agreed that @taldcroft's input would be useful here, but I think it would be nice to have the greenwich part in here for 0.3 - we already had a user asking on the mailing list a while back about getting sidereal times, and given you've done most of the work here of connecting to ERFA, it makes sense to get the uncontroversial part in now.

And @taldcroft , what do you think about the plan above to reduce this to just the greenwich part? I agree with @mhvk that this shouldn't go in for 0.3 if it has the LST, but greenwich shouldn't be any issue because it doesn't require location information...

@eteq - I agree with the plan to reduce to the Greenwich bits for this PR in 0.3.

@mhvk - could you rebase this and address @eteq and @taldcroft's comments?

@astrofrog - I think this should be postponed to 0.3.1. While I can easily do the practical items, I'm still not convinced about the requested name changes and the necessity to drop local sidereal time pending a still-to-be-implemented Earth coordinate class. Furthermore, I really would like there to be a proper test of the implementation, against a known-correct implementation, and this also needs an update to the documentation.

@mhvk - note that that actually means it has to be postponed to 0.4/1.0, as the 0.3.x series is only bug/docs fixes, no added features.

I still think we should put in the greenwich stuff only, but I probably won't have time to do it myself by Friday. And I see your point that its important there be tests for this, so it will have to be triaged if you don't think you can get to it by then.

Also, just to be clear, gmst and gast are non-starters for public API functions, given our coding guidelines. But there's certainly room for deciding exactly what's a better name if you don't think what I suggested is suitable.

@mhvk - do you think you can address @eteq's comments today?

@astrofrog - as I indicated above, I neither agree with @eteq's comments, nor think this PR is complete, so just postpone.

@mhvk - this looks fine to me, but my voice shouldn't be too loud since I don't actually ever use sidereal time. ;-) For instance is mean what gets used > 90% of the time (which might justify a default)? I just don't know.

For instance is mean what gets used > 90% of the time (which might justify a default)? I just don't know.

In most of the cases I used sidereal time, the difference was not relevant. But then, for many cases when people use Time, the difference between UTC and TAI is probably not relevant either. Best for now may be not to have a default; it will certainly help people know what is happening when they read someone else's code.

@eteq - do you think the above proposal(s) address your concerns?

Yes, this plan looks great to me. I also agree that requiring a type is good for now - as you say @mhvk, I think it doesn't matter in most cases, but when it does, often assumptions are made. So forcing people to think about it is a good thing. If we get a lot of complaints we can always add in a default in a later release.

A few suggestions that are just improvements to this main idea:

• Perhaps it should be called sidereal_time instead of just sidereal? After all, it could be a sidereal period, or a synodic-to-sidereal conversion or something, based just on the name.
• For longitude, I would propose this for now:

longitude :  `Quantity`, None, optional.
    If a `Quantity` , this must have angular units (includes `Angle` or `Longitude`) 
    and will be treated as the longitude on the Earth at which to compute the sidereal time. 
    If `str` it should specify the observatory.  Currently only "greenwich" is supported 
    (equivalent to "0 degrees" in the `Quantity` form), but in the future other observatories 
    will be added.  If None,  it is the `lon` attribute of this Time object.

That is, no floats are allowed, because of the unit ambiguity, and we can get greenwich in now, as it is an IAU standard above and beyond other observatories, and this lets us deal separately with implementing other observatory locations (I like @taldcroft's idea here, but it is tied up with the coordinates stuff and we don't want to delay implementing this just for that.). It also gets rid of bools in longitude, because that's just inherently unclear without looking it up in the docs.

So this would adjust @taldcroft's example to usage like:

t.sidereal('mean', 'greenwich')
t.sidereal('mean', 0*u.degree)  # same as above
t.sidereal('mean', -155.4750*u.degree)  # Keck's longitude by hand
t.sidereal('apparent', 45.*u.degree, 'IAU1984')  # apparent ST at lon=45., 1984 prec-nut
t.sidereal('mean', 0)  # could also allow this as a special-case "easy" way to get greenwich, as its the one case where the unit doesn't matter.

@eteq, @taldcroft - OK, it seems like we converged; I do like this substantially better! I think I also got (al)most all of the other comments, including, most importantly, reproducing the test cases in t_erfa_c.c. Once any possible remaining concerns are addressed, I'll rebase (to get the updated Time docstring if nothing else) and squash the earlier commits.

@taldcroft, @eteq - I now rebased and also updated the documentation (including the time-scale figure). Let me know what you think.

@taldcroft, @eteq - I rebased sidereal time again, and fixed the few documentation issues that were preventing a clean pass from travis. Let me know what you think of the approach (and the documentation).

My comments above are fairly minor, so once they're addressed I think this is set as far as I'm concerned.

@eteq - thanks for the comments. One question related to using 'radian' rather than u.rad. As it is, I tried to avoid using units directly so that the Time package stayed as independent as possible from other parts of astropy (I only import units locally in TimeDelta.to(), where of course it is needed).

Is there a policy for this? I personally prefer keeping things separate, if only to avoid circular imports, but units is becoming such a basic package that I perhaps should not worry. If you think units should just be imported, I'll do a separate PR on that, as there are also several instances where 'day' is used when it could have been u.day.

First, regarding @mhvk's question on the units syntax: I'm not sure I'd call it a "policy", but when we introduced Unit in the first place, it was the intent to use the u.rad syntax in library code, and reserve string usage as a shorthand for use with interactive or quick science code, or for actual parsing of input text. This is partly a stylistic choice (which is of course open to interpretation), but more a performance thing (which is not): using the string parser can be 2-10x slower than the object-based syntax, because the parser has to be fairly general-purpose.

As for circular imports, we've been using units at the top-level of a lot of subpackages, just as you said- we already had to do some tricks with this to make constants work, so the infrastructure is there to address these issues if they pop up again. So I wouldn't worry too much about that, but you could can always get around it by instead putting from .. import units as u in the function instead of at the module level (as long as you're actually using it inside a function).

@eteq - OK, that makes sense. I'll go ahead and make a separate PR where I pull units out.

I think with that this PR is ready to merge, correct?

As far as I'm concerned it looks good. @taldcroft, do you have anything to add, or should we go ahead and merge this?

@eteq, @taldcroft - the follow-up changes for putting units up front are ready [1]; I'll rebase and make it a PR once this is merged.

[1] https://github.com/mhvk/astropy/tree/time-import-units-on-top

Looks OK to me.

Following comments by @astrofrog elsewhere, I added a PR number to CHANGES.rst; feel free to merge.

As always, thanks @mhvk!