@mhvk - I would be inclined to drop lon and lat entirely in favor of location (which also implies not adding height). Of course this needs to be done with a deprecation release. Even though we are trying for API stability, I suspect that lat and lon are not being used that much yet for Time, and we should not accumulate this mess of overlapping keyword args in the API.

I would say that location should accept a location object or any tuple that can reasonably instantiate an EarthLocation object (e.g. two angles => lon, lat; two angles and a length => lon, lat, height; three lengths => x, y, z).

@mvhk: yes, I think that this makes sense. And I'm also fine with having it accept a Location object rather than lat/lon/hgt.

This seems like it would be useful beyond internally, though, so maybe it should be available outside of the Time routines? I would use it for the observatory positions in PINT if that were the case.

I agree that EarthLocation should live outside of time. Since it's basically a coordinate it seems like it should live in that package.

@taldcroft - I agree that it may be best to just move away from lon and lat in favour of location, so have implemented that suggestion, as well as others by you and @scottransom. Also updated the tests, etc.

It also makes most sense to make the EarthLocation class not Time-specific, but I'll wait for comments from @eteq and @astrofrog before moving it (and before updating the documentation).

Just to let you know, @mhvk, this is on my radar to review (along with #1927), but it might be a few more days yet before I get to it (holidays and all)...

@eteq - no worries; is good to get this at least in the right direction.

As the new location class is based in Quantity, the correction for the erfa problem with output being in 'm' and input being in 'km' (#1929) was rather trivial to solve. It does beg the question whether our interface to erfa should handle quantities generally, so these types of problems do not recur. Added a note to #1453 to that extent (and do the conversion to "km" inside the erfa function call, as a reminder of why this is necessary at all)

@demorest - on the e-mail you sent separately:

The only thing we are directly using ERFA for in PINT is computing observatory positions/velocities (ie turning ITRS xyz into ICRS xyz). This stuff would probably be a nice thing to put in your EarthLocation class in astropy.

I think this would indeed be great to add, though it sounds like this is more properly done as part of coordinates (where the EarthLocation class should move anyway). Could you point me to where in PINT this is done, so we can see what it would entail?

cc @eteq: this is about which erfa routines are needed for pulsar timing besides those in erfa_time; of course, also directly relevant to #1453.

@mhvk - look at this pint source file: http://github.com/demorest/PINT/blob/master/pint/erfautils.py

It's not too complicated, and I think it very closely follows another one of the SOFA usage examples (maybe @scottransom remembers where to find that in the SOFA docs).

@taldcroft - regarding your comment about this needing to live in astropy.coordinates (which I agree with) do you think that in the long term, all of astropy.time should also live in astropy.coordinates? After all, time is just another coordinate?

@astrofrog - am not sure coordinates is necessarily the best place (though better than Time!). An alternative would be to separate out the mathematical coordinates (Angle, Longitude, Latitude, CartesianPoints, etc.) from sky and surface-based coordinates. Another thought would be to have an astropy.observatories or astropy.earth subpackage. (Indirectly related: we also should still move iers out of utils; the agreement seems to have been to make this an upper-level package as well, possibly as part of earth; see #1620).

@astrofrog @mhvk - I do not think time should live in coordinates. While @astrofrog is right that unifying them seems attractive from a physics/relativity standpoint, there still a conceptual difference. When someone asks "how do I convert from ISO time stamps to MJD?", I think almost no one will find it natural if we say "it's in the celestial coordinates subpackage." They'll definitely have to be rather interconnected, but I don't think it makes sense to actually make them the same.

As for where Observatory should go, I see @mhvk's point that something like astropy.observatories makes sense - in the future that might also include information like telescopes and their limits, instruments, or whatever. And maybe also tools like observing charts belong there. All of the actual location stuff (e.g. an EarthLocation class) should definitely live in coordinates, though - they would be part of the coordinate transformation graph, after all.

👎 on Time in coordinates for the reasons @eteq mentioned. The fact that there is a certain cross-coupling in terms of usage doesn't mean they have to be in the same top level package. I also (still) agree that EarthLocation definitely belongs in coordinates.

I don't see a big benefit to splitting out mathematical coordinates like @mhvk suggested, that would just add confusion in terms of where to import relevant classes etc., not to mention a huge API change when we've been promising that coordinates will settle down.

@eteq - I moved EarthLocation to coordinates (earth.py). I know I should also add test cases for the class itself, as well as documentation to coordinates, but would like to be sure you think the overall concept is solid before I do so.

Note that I kept the erfa interface routines in Time, since those are better dealt with as part of #1453.

I was looking through some of the backlog and realized this is still hanging. @mhvk, do you think we should hold off on this until we get through APE5 implementation? It may be that we'll need to move the ERFA stuff as part of that, in which case it might be awkward to merge this in beforehand... But it might also just be a matter of rebasing before merging APE5. (although this also needs a rebase now.)

@eteq - I'd be in favour of putting it in, as long as everybody is happy with the overall API (and it would be hard to think how else one would initialize it that from lon, lat, height or x, y, z). I'll rebase.

OK, I had assumed you wrote a wrapper around CartesianPoints to mimic the CartesianRepresentation API. Should've looked at the code.

Aside from that one minor bit it seems fine to me. @taldcroft, did you want to actually look more, or is this good to go now as far as you're concerned?

Just to check, currently what is passed to location is assumed to be an EarthLocation - is the idea that in future we could have non-Earth locations but that by default if one only passes a tuple it is interpreted as an Earth-based location? This is fine, but just want to check that is that long-term plan?

I was just scanning the tests and noticed there doesn't seem to be any explicit use of EarthLocation anywhere in the tests. There should probably be at least one test of actually passing in an EarthLocation object for location.

OK, I'm done with my review!

@astrofrog - Not sure about non-Earth based locations, since many timescales do not make sense off the Earth, but I certainly hope that we can pass, e.g., observatory names (or classes, which might be subclassed from EarthLocation).

@taldcroft, @eteq - Good suggestions. All implemented.

This is good from my perspective.

@astrofrog @mhvk - I think it definitely will make sense in the future to allow something more than just EarthLocation... And once we have the IAU2000 stack in, that will be a lot easier because we can use those transforms.

But the question, now that @astrofrog brought it up, is this: should we drop the tuple form entirely, and only allow EarthLocation? I'm actually 👍 to that, as it's a lot less ambiguous... what do you think, @mhvk ?

I do think that it is probably good that at least some non-earth-based locations are handled. I'm thinking specifically of satellite observatories, which often use UTC-based timescales and then do conversions to TDB etc as appropriate....

@scottransom - I think there is nothing here that would stop us from expanding to satellite locations, especially if one has their geocentric coordinates.

@eteq - I can see the point of requiring an EarthLocation, but somewhat like the convenience of not having to import it... Also, eventually we hoped to do something like location='cfht', which would also requiring the ability to pass on what are essentially initialisation arguments.

I think I'm ok with strings and tuples being a convenience for earth locations, but I guess I just meant that I wanted to make sure that we make it clear tuples get passed to that class and are just a convenience.

@astrofrog - OK, that makes sense. I looked at the PR again and think over the various iterations all documentation has become fairly clear that it is an EarthLocation or something that initialises one; I only changed the entry in CHANGES.rst to make that clear as well.

@mhvk - sounds good! Once the above typo is fixed we can merge this!

To add one item for the future, @scottransom : I think this will be pretty straightforward once we've got the full IAU2000 stack working, which is the next step after APE5 is done (and shouldn't be too onerous once we figure out the easiest way of getting all the ERFA parts to talk easily with astropy).

And nice work on this, @mhvk : took a while, but I like how it turned out!

Looking at EarthLocation in the API docs, there's a few minor things in the docstrings that should be cleaned up:

• The longitude and latitude attributes are called lon and lat in a few places (and cross-links in the html output don't work).
• The following attributes should have one-line descriptions: ellipsoid, height, latitude, longitude, x, y, z
• In the from_geocentric, from_geodetic and to_geodetic method docstrings the first sentence should end with a dot ., so that they format correctly in the API docs method list.
  There should also be a dot . after "Location on Earth" on the class docstring.

@cdeil - OK, will issue a new PR for those. One item that requires a bit of thought (put here, so people can comment), is whether it makes sense to initialize longitude, latitude, and height with the short forms in from_geodetic(lon=..., lat=..., height=...) and have long-form attributes. Now that this is not yet in use, shall I change to short or long form in both? My tendency would be to go for the full names also for the arguments.

@cdeil - see #2346.