Coordinates API, take 2

[eteq]

This is a re-working of the coordinates API intended for v0.4 . It is written to be a "compromise" API to address a variety of problems and suggestions that have come up since coordinates was introduced in v0.2 . It is a companion to APE5, which will be submitted shortly.

Note that there are a number of places that have the text OPTION - these are items where it was not clear if an optional addition is worthwhile or not, so feedback on those, in particular, would be helpful. While they are mostly relatively minor, there is one possibly major architecture choice: whether parsing of single strings with both ra/dec should be included in the low-level classes, or just the high-level class. See the document for details on this, but it's definitely a decision that needs to be made.

[taldcroft]

@eteq - congratulations on getting this and APE5 out! From my 10 minute skim this morning it looks great and I had only a couple of minor questions, but I'm sure there will be more as the discussion gets rolling.

[taldcroft]

This broadcasting of distance is good. One might go further treat all the inputs lat, lon and distance on an equal basis and broadcast them all to the highest common shape. (And raise an exception where this isn't possible). Having one common direction and several distances is not an outlandish scenario, but in any case this is a minor point.

[eteq]

A very good point - that should be trivial to implement with np.broadcast_arrays. Will add a note about this in the version I'll push up in a bit

[astrofrog]

@eteq - I will review this over the next couple of days, but just a minor esthetic request - some of the lines are very long, could you space it out more vertically and include the comments maybe before the lines rather than inline (since the code lines are already long).

[taldcroft]

Minor point, but this is included in the string-input option block, but store_as is a different question. In fact I get the impression this is not a decision, but instead an illustration of a new keyword arg store_as that definitely has a need.

[eteq]

I put it in the option block because the idea is that you don't necessarily need it if Representations don't do any string parsing beyond what's in Angle. The key point is that Representations accept normal angle objects. That is, the equivalent, with no need for store_as is:

c2 = coords.SphericalRepresentation(Longitude('5:10:20.52', u.hourangle).to(u.radian), Latitude('+23:23:23.5', u.degree).to(u.radian))

Or do you (or @mdboom) still think store_as is needed in this case?

[mdboom]

I can see how store_as might be more convenient in some cases, but it doesn't seem strictly necessary. Despite usually being on the side of "only one way to do it", I think in this case, we should have a store_as here.

[eteq]

Alright, I'm leave this in but with a non string-related example.

[taldcroft]

I don't see any specifications of the class signatures. In particular, will there be class-specific args like lon and lat as the initial positional args, or will the first three args be generic, or will it be *args?

It makes me unhappy to see function signatures with too much overloading, e.g. having a first argument named lon which actually can take a full coordinate or any number of other options. I guess this happens (e.g. in Time), but it would be nice to make this clean. I guess a comprimise is something like *args, lon=None, lat=None, ..., where there is an explicit declaration that positional args are allowed, but the named keyword args are also there.

[eteq]

I'll try to work my way through comments soon, but one logistical item: @astrofrog, I see your point about shorter lines, but if I do that now, it will hide any of the inline comments on those lines. So I can either do that while going through existing comments and have you look over after that, or you can give comments on the current version and I can clean up the lines after you've looked through these - either way is fine with me.

[astrofrog]

Typo: convenience

[astrofrog]

There are a few more below, so maybe just search and replace.

[astrofrog]

@eteq - apart from my comments above, one last thing I'd like to see done if possible is to improve PEP8-compliance of this document. I'm probably obsessive, but I find it hard(er) to read comments that start #this is text, and PEP8 suggests always including a space after the #, so if you have time, it might be good to do this. But since I suggested it, I can also just do it in master once this is merged ;)

[mhvk]

@eteq - finally had a look, and overall I think it is great! I added some comments, one in part asking whether this would resolve astropy/astropy#1850, the other about the need for store_as, and the final one about whether frame_attr_names could not be a dict or even implicitly defined in __dict__.

For the representation classes, I would argue for having an explicit initialiser, e.g., SphericalRepresentation(lon, lat, r), but being somewhat liberal in how things are interpreted so that the behaviour is as you write in the API. I think this is most consistent with other parts of astropy like Quantity and Time where the arguments are named (val, jd1, jd2), but can, e.g., contain a class instance (I realise a possible consequence of the API allowing SphericalRepresentation(Latitude(...), Longitude(...)) would in my scheme mean no exception would be raised on SphericalRepresentation(lon=Latitude(...), lat=Longitude(...)). I could live with that, or with disallowing both (as you wrote elsewhere, people can subclass and define things differently; I think it is fine for the low-level classes to be stricter).

Finally, in the end we will need to add proper motions. While we should not do that here, it may be good to think whether the current API would allow adding them. My sense is that it does, that for each it will be possible to add a ..., pm=(val1,val2,val3) parameter, where val1, val2, val3 have to have units consistent with time derivatives of the values used for the representation. Of course, it may be better to have subclasses, in which case this point is moot. Anyway, I bring it up only to say that I do not think this is a reason to hold this back.

p.s. With this implemented, it should also be trivial to adapt astropy/astropy#1928 and have an EarthLocation class!

[adrn]

@mhvk If you're going to add proper motions, then it should also support also radial velocities :). I don't know how I feel about having kinematic information in the same object -- maybe someone else has though about this more but we should probably have a discussion at some point about how to best handle this. I do agree with your point that it shouldn't hold up this PR though!

[astrofrog]

Proper motion is also important to include in astropy.coordinates because it matters for some coordinate transformations, specifically FK4. For things like CartesianRepresentation we should basically store x, y, z, and vx, vy, and vz (which then represents both radial and proper motions).

[mhvk]

@adrn - indeed, the 3rd value of the proper motion tuple would have to have to be in km/s. As said, though, I don't think the API excludes adding this later, so it should be fine. In the end, in the classes, it may boil down to something like if self.pm: <do things with proper motion>.

[astrofrog]

@eteq - let me know once you've had a chance to address the various comments above, and we can then send it out to the list for a final coupe of days of review before we merge.

[eteq]

Sorry this has been languishing for the last week. I'll go through and fix up the typos/PEP8-compliance @astrofrog pointed once the questions above have been answered, as some of those require a bit more clarification.

On the question of PMs and RVs (@mhvk, @adrn, and @astrofrog): I envisioned those be adding later, just as @mhvk said in his last comment. (I'm also thinking about a quite different approach where PM/RV objects generate low-level classes instead of being contained within them). But I don't want to get mired in that discussion right now unless it's crucial - that is, if the current API will actually prevent PMs and RVs from getting added later.

@astrofrog - hopefully another email to the list won't prompt another round of comments and delay things further... But I suppose you're right it should be done ;)

@mhvk - I agree this will make astropy/astropy#1928 quite a bit easier, but I actually think we may not want to do it right away: once this is implemented, the road is clear to adopt the IAU2000 coordinate stack, and the "lowest" layer of that is ITRS, a much more formal/accurate way of defining EarthLocation. But that probably is better discussed in astropy/astropy#1928 or a follow-on.

[mhvk]

@eteq - apart from the question of strictness in the initializer, I think this is all set.

[eteq]

Alright, I think I've got almost everything in the commit I just pushed up, but a few small items remain. @astrofrog - if you want to send the message, or just merge after a day or so, you might do so because these are fairly minor:

• Do we still need store_as? @mdboom mentioned wanting it, but @mhvk said he didn't think it was necessary.
• @astrofrog - you had expressed a concern about the fk5_j2001_with_data example (new line 218), but I wasn't sure what you found confusing in that section. I added a little bit to clear up what was going on there - let me know if it still doesn't make sense.
• I tried to synthesize @astrofrog, @Cadair, and @mhvk's opinions on whether the representation initializers should be "magic" in accepting various inputs in different orders. The current version is SphericalRepresentation(lat=None, lon=None, distance=None, reprobj=None, copy=True). If either of you still don't like that, feel free to protest.

[astrofrog]

@eteq - here are my comments:

• store_as - if the 'low-level' classes can accept e.g. [1 * u.deg, 3 * u.hourangle, 3 * u.arcsec] then we need store_as. If they can only accept quantity objects (scalar or array), then we don't need it.
• fk5_j2001_with_data - makes sense now1
• Just curious - why lat then lon rather than lon then lat? In astro, we mostly use (lon, lat) right? I don't really mind either way, just interested.

Otherwise this looks good to go!

[mhvk]

Comments on the comments:

• for list input, I think if one wants to have anything but the first unit (which Quantity defaults to, one should do, e.g., lon=Quantity([1 * u.deg, 3 * u.hourangle, 3 * u.arcsec], u.radian); I feel store_as just duplicates this unnecessarily.
• definitely have the order be lon, lat!

[eteq]

@mhvk @astrofrog - To explain lat, lon: While it's true that Equatorial systems (and many celestial systems) are lon,lat/RA, Dec, Horizontal coordinates are lat,lon/alt/az, as are the related systems that are in geocentric frames. So there's not an absolute best answer. And in a more generic context, one often speaks of "latitude and longitude" rather than "longitude and latitude", so it seemed like lat,lon made sense given that it was for the generic spherical system. (remember, the low-level classes like ICRS or FK5 have ra and dec keywords, anyway.) Given that it's rather arbitrary, though, and it's currently 2-against-1, I'll chgange it to lon,lat unless anyone else voices an opinion.

On store_as, I think I agree with @mhvk that Quantity([1 * u.deg, 3 * u.hourangle, 3 * u.arcsec], u.radian) does indeed make more sense given that we're already requiring Quantity to provide the unit information in the first place. We should definitely include an example of this in the docs, though. @astrofrog, does that sound ok to you? (And @mdboom?)

[astrofrog]

👍 to both suggestions!

[eteq]

Alright, I've taken care of both of those, although of course I can revert them if anyone has objections.

[Cadair]

I for one am 👍 merge.

[astrofrog]

No objections have been raised, and this looks good, so merging!

[Cadair]

Woop woop!!