Add CartesianRepresentationFrameAttribute to GCRS and PrecessedGeocentric frames - #5207

I should mention it would be nice to merge this ASAP, since astropy/astroplan#167 introduced some fairly dodgy hacks to workaround #4926. It would be nice to remove those before the next version of astroplan is released.

@StuartLittlefair - This looks good, except that I think we should not aim to be flexible in where the three coordinates are located: I think they have to be the first dimension, as for CartesianRepresentation itself. Otherwise what does a (3, 3) input mean?

p.s. Agreed it would be nice to merge this soon -- it will also help make baseframe shape-aware.

Also, this does need a changelog entry. Is this something that makes sense to backport? My guess is not, so I put the milestone at 1.2.2 for now.

p.p.p.s. The failures seem real.

Ok @mhvk - I can see that (3,3) is ambiguous. It's a one liner to change this and most code within astropy shouldn't need to change. There may be some potential changes to get_gcrs_posvel which is publically exposed. It might be better to leave that as is, and reshape the returned quantities before creating GCRS frames from them. This is a little clunky though!

I'm not sure about backporting. It would be useful for astroplan to backport to 1.2 I think, but I don't fully understand the implications.

I'll have to look at that and the failures tomorrow morning.

@mhvk good call on removing the flexibility. Everything "just works" regardless.

The only user-facing change as a result of this PR is that accessing the obsgeoloc attribute now returns a CartesianRepresentation so someone may have to replace coord.obsgeoloc with coord.obsgeoloc.xyz in their code, but that's the only outward change I can see.

@StuartLittlefair - great that it "just works"!! I have one in-line question for simplification. Another is a more general one: why would obsgeoloc at least not use EarthLocation? (for which there is already an attribute).

In the end, I don't think it matters too much (indeed, EarthLocation should probably start using CartesianRepresentation inside...), and probably I should just read the code better to understand what these attributes are for...

In any case, it would probably be good if some of those who wrote the code can have a quick look as well, so cc @taldcroft, @eteq.

Thanks for the suggestion @mhvk . I was quite impressed by how simple it was. Almost as if someone thought carefully when they wrote these objects in the first place ;)

One reason not to use EarthLocation is that obsgeoloc is a coordinate in the GCRS frame, whereas EarthLocation is in a different frame (ITRS?). If we had an EarthLocationFrameAttribute it would require a transformation every time we needed the GCRS coordinate.

Also OK on EarthLocation -- at some point I just have to bite the bullet and try to understand the differences between the difference frames...

This PR almost addresses part of #5209. Instead the example there causes a different bug:

In [4]: SkyCoord(moon)
Out[4]: 
<SkyCoord (GCRS: obstime=2014-01-20 22:00:00.000, obsgeoloc=(1712675.0, 5328049.0, -3053885.0) m, obsgeovel=(-388.0, 125.0, 0.0) m / s): (ra, dec, distance) in (deg, deg, AU)
    (172.4, 0.51, 0.002671)>

In [5]: SkyCoord([moon])
Out[5]: 
<SkyCoord (ICRS): (ra, dec) in deg
    (172.4, 0.51)>

In [6]: SkyCoord([moon, moon])
Out[6]: 
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, AU)
    [(172.4, 0.51, 0.002671), (172.4, 0.51, 0.002671)]>

In [7]: SkyCoord([moon.icrs, moon.icrs])
Out[7]: 
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, AU)
    [(122.91386538, 19.98343108, 0.98321877),
     (122.91386538, 19.98343108, 0.98321877)]>

In [8]: moon.icrs
Out[8]: 
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, AU)
    (122.91386538, 19.98343108, 0.98321877)>

I think this is because SkyCoord([moon, moon]) is ignoring the frame of the SkyCoords in the list.

Thanks for the comments @taldcroft

The last few commits should address them, I think.

Fixes #5216

Overall this is looking close. I just looked at the test diffs and it does not appear that all of the logical code paths are being tested. I'm guessing that existing tests won't cover this, but I haven't explicitly looked.

I've added a few extra lines to the regression test which should test all the logic I added.

@StuartLittlefair - I like the idea here and I think this is basically good (it looks to me like you had no more changes pending, right?).

But I have one concern: backwards compatibility. If I understand right, this is still backwards compatible at the input - that is, if you provide an obsgeoloc as sc = SkyCoord(..., obsgeoloc=[1,2,3]*u.km,...) it still works... but if I do sc.obsgeoloc I get something different now, so any code written to work with quantities no longer works. I'm reading that right, correct? So the question is: can we think of any way to maintain the ~ standard practice of having at least one version of deprecation for API changes? One option might be to use new names (obsgeoloc/obsgeovel are not great, anyway... although I have no immediate suggestions because I wrote them originally), have those be the real attributes, but have obsgeoloc and obsgeovel translate back to Quantity while raising deprecation warnings.

Alternatively, if we think these attributes are never actually accessed in the wild, then maybe it's not worth worrying much about it. But it should still definitely be listed in the API changes section of the changelog.

Now back from holidays and had a bit more of a look. On the backwards compatibility, possibly the direct use of the attributes is a rare enough case that we can just do it rather than deprecate?

I ask in part because at a bit higher level I wonder a bit about the logic of having a separate loc and vel: logically this is really an EarthLocation or SkyCoord that includes proper motion and is set to a particular reference frame. I think eventually we might want to move to a name that reflects that (perhaps just location as for the altaz frame). But it might be good not to move more than once!

Above, @StuartLittlefair mentioned that one perhaps shouldn't use EarthLocation directly since then every time one needs to convert; but if so, we might want to store this as the combination of the two that is actually used in the calculation (i.e., erfa's pv array).

Anyway, it is all leaving me a bit confused. However, from the perspective of allowing proper broadcasting between coordinates and their frame attributes, it certainly is very helpful that with the current implementation, the shape attribute is correct (without the 3 for the x,y,z part).

p.s. Separately, EarthLocation has a get_gcrs_posvel method. I think this will have to be changed to return a tuple of CartesionRepresentation as well. Which of course changes the API as well...

@mhvk, @eteq:

Thanks for the comments. A few thoughts:

• I feel the question about whether to deprecate or not is slightly above my pay grade. However, my feeling is not to bother. I think doing sc.obsgeoloc is probably a very rare use case. Moreover, the change required in any code would be to replace sc.obsgeoloc with sc.obsgeoloc.xyz, which is not major.
• If we do deprecate, what about making attributes which shadow the main ones, so _obsgeoloc, for example? These would actually be CartesianRepresentations so would maintain the shape advantages for broadcasting. We'd make obsgeoloc a straight property, whose getter raised a deprecation warning. Then when we finally remove the deprecation warning, changing the codebase is a matter of replacing _obsgeoloc with obsgeoloc.
• On the issue of moving away from separate locations and velocities I feel this is something to revisit when the challenge of implementing proper motion is addressed. Until then, the way it is done now gives you the flexibility to make GCRS frames for orbiting satellites etc if you want to do that.
• Finally, addressing get_gcrs_posvel. I thought about changing this for consistency but was in two minds. In the end I decided not to change it, since the output from get_gcrs_posvel can be supplied as is to initialise a GCRS frame and it minimised the API changes. Should I revisit that decision?

Will add an API changeling entry, but pending a decision on deprecation, that is the only outstanding change.

Thanks @mhvk. This is now passing.

Pending some consensus on deprecation I am finished working on the PR.

To have something to look at, I changed EarthLocation.get_gcrs_posvel to return CartesianRepresentation as well. Somehow, I couldn't make a PR to this branch (no @StuartLittlefair branch to send the request to, very odd), so I made a new PR (where I also squashed all the commits here into one, and rebased). Please have a look at #5253.

Am closing this since it is part of #5253, and there seems to be agreement that it is better to change EarthLocation.get_gcrs_posvel as well.