Make AstrometricFrame behave like other coordinate frames.

[alexrudy]

This is a stub trying to make the AstrometricFrame behave like @taldcroft suggested in #4931.

Some things are still a little bit wonky:

• Why can't I switch the inheritance order of AstrometricFrame and framecls (so that AstrometricFrame is first, which I think makes more sense in this case)? When I do this, the FrameMeta doesn't seem to pick up on the representation info from framecls.
• Is there a way to inherit the frame attributes from AstrometricFrame in _AstrometricFrame
• If there is a way to inherit the frame attributes, is there a way to just change the CoordinateAttribute's internal _frame variable? In this case, I think it would have to live on the class and not the descriptor, and the convert_input function on CoordinateFrame does not have access to the AstrometricFrame class, so maybe this is too complicated.
• Should AstrometricFrame always be called AstrometricFrame, or should it incorporate the name of the base frame class?
• Also, still needs some minor doc fixes and a changeling entry.

[alexrudy]

CC: @taldcroft @eteq @mhvk and @cdeil who all discussed this API earlier.

[mhvk]

Can this ever happen, given that AstrometricFrame is always a subclass of BaseCoordinateFrame?

[alexrudy]

In fact, because BaseCoordinateFrame doesn't implement __new__, this is always triggered right now. If another frame were to implement a custom __new__, this would be skipped by the parent class when traversing the class hierarchy if AstrometricFrame came before the framecls above, which ideally it would, but that breaks something about ordered descriptor that I don't understand (but maybe @eteq could fix?)

Regardless, right now, this is required to prevent this __new__ method from breaking should an descendant implement __new__ in the future. When this happens, if we didn't put in this statement, arguments would never get passed to a subclass __new__, since we'd need to assume we are calling object.__new__. As this is kind of a subtle bug when that happens, I would advocate for leaving this check in now.

[mhvk]

OK, yes, I was sloppy in looking through the chain. Given that object is one up as far as __new__ is concerned, it definitely makes sense to have this in.

[mhvk]

This looks rather nice!

[cdeil]

Change icrs_to_astrometric to reference_to_astrometric here?
This was a suggestion @taldcroft made in the other issue and I also found this confusing to name this icrs when it's not specific to icrs.

[alexrudy]

I've tried to address @cdeil 's comments w/r/t documentation.

I have some remaining questions which are in the description of this PR, about the internal workings of coordinates that I'd like to address before being totally comfortable with this PR, maybe @eteq can help.

[eteq]

I need to spend some time to wrap my head around this, which I will try to do soon... But to address my biggest initial concern:

Should AstrometricFrame always be called AstrometricFrame, or should it incorporate the name of the base frame class?

I emphatically say it should - e.g. AstrometricICRS. Otherwise it seems like they are all one frame, when in reality they are genuinely distinct frames. It's less obvious that this matters in the "on-sky" situation, but when you start considering the distance coordinate (and the fact that at some point we will want to support velocities), it's clear that you can't just think of it as "an abstract coordinate centered on something".

And that's why I ended up doing the whole metaclass magic in the first place: if you change the __name__ of a class after the fact, it does not propagate to all the places it needs to go: it has to be set in the metaclass (which is why the signature of a metaclass __new__ is name, bases, dct - you need to be able to fiddle with the name in the metaclass if you want to change the name). I think you can probably still do that in this implementation... But I'm not 100% sure yet.

[alexrudy]

@eteq I'm still using your metaclass under the hood, so it is easy to have the class name change, it just pushes a little bit against @taldcroft's initial idea behind the API.

I actually found that I left the class name changing while debugging so that I could see what was happening, then removed that part for the final version (maybe a good sign that the class name should change).

From the original API request:

# Now: you never see something that looks like a CamelCase frame class
my_frame_cls = make_astrometric_cls(origin.frame)
my_frame = my_frame_cls(origin=origin, rotation=20)
coord = SkyCoord(1, 2, unit='deg', frame=my_frame)

# Proposed: this *looks* more like the rest of the coordinates API
my_frame = AstrometricFrame(origin=origin, rotation=20)  
coord = SkyCoord(1, 2, unit='deg', frame=my_frame)
my_frame.__name__ # this could still be AstrometricFrameICRS or whatever

The tricky part is that we don't want to confuse users too much either way. In this new API, AstrometricFrame will always return a subclass of AstrometricFrame with a new name. This breaks some basic assumptions, like AstrometricFrame(origin=my_origin).__class__ is not AstrometricFrame but issubclass(AstrometricFrame(origin=my_origin).__class__, AstrometricFrame) still holds true.

[taldcroft]

I emphatically say it should - e.g. AstrometricICRS. Otherwise it seems like they are all one frame, when in reality they are genuinely distinct frames.

How is this situation different from AltAz? In this case you have a frame whose orientation is a function of two instance attributes (location and obstime). In the case of Astrometric it is basically the same idea, but just origin and rotation. So I really don't see why the frame of origin needs to be pulled out into the class name since we don't require having AltAzKPNO or whatever.

On the question of the frame that defines the coordinate origin for distance and velocity, what could be more natural than myframe.origin.frame?! This can be easily documented in the class docstring in a line or two. This seems actually better than encoding that origin frame as text at the end of the class name since there is then no clean programmatic way to access that frame. In other words if all I know if that the class is AstrometricAltAz, how I get the AltAz class from that? Much better to do coord.frame.origin.frame than munging coord.frame.__class__.__name__.

I'll say that the first time I saw AstrometricAltAz my head was spinning (just like AltAz) trying to make sense of what that means. I think that we should aim the API at the 95% use case (what @cdeil originally with FOV frame) and make it nice and clean for that case. For experts (devs) we can document that the class is a little funky under the hood and they need to be aware of the slight idiosyncrasies.

[eteq]

@taldcroft - actually, what @cdeil's original use case as communicated to me was exactly this: he needs both the AltAz oriented "FOV frame" and the Ra/Dec oriented frame (centered on the same point). So in his use case there'd be two things in use floating around called AstrometricFrame. I sympathize with the 95% of use cases perspective, but I don't think it's obvious which is more "typical" (oriented relative to ICRS, relative to AltAz, or relative to something else).

More to the point, though, I agree wholeheartedly with @alexrudy's second statement:

In this new API, AstrometricFrame will always return a subclass of AstrometricFrame with a new name. This breaks some basic assumptions, like AstrometricFrame(origin=my_origin).class is not AstrometricFrame but issubclass(AstrometricFrame(origin=my_origin).class, AstrometricFrame) still holds true.

"hiding" the fact that it's a subclass will just make users even more confused if they do anything beyond the basic cases outlined here (and if they just do the basic case, it doesn't hurt them any). Beyond @alexrudy's point, there's the additional bit that this really has to create a new frame anyway for the transformations to work, so if you were to, say, plot up the transform graph now, you would see lots of different frames called AstrometricFrame. But if you let the name change, they say things like AstrometricFrameICRS. And that's exactly what makes them special: is contrast to e.g., AltAz, the astrometric frames really are tied to their origin's frame because that's the only way to transform into them.

TL;DR: I'm fine with this approach, except with the modification of putting the name of the frame back in as @alexrudy indicates can easily be done in #4941 (comment)

[eteq]

Oh, and I'm noticing the other remaining checkbox from @alexrudy at the top: I think you're asking if the frame attributes from origin should be included in the AstrometricFrame, right? I think no, if I understand you correctly: they are already embedded in the frame attributes of origin, and we don't want to duplicate them.

[alexrudy]

@eteq I agree origin's frame attributes should stay living on the .origin frame.

I was actually asking about the re-implementation of origin itself (the CoordinateAttribute) for each subclass (see https://github.com/astropy/astropy/pull/4941/files#diff-46639dd9197a682e9c18b4ff5900645cR59 vs https://github.com/astropy/astropy/pull/4941/files#diff-46639dd9197a682e9c18b4ff5900645cR161). The base class (AstrometricFrame) has an .origin attribute, and so does the subclass. The subclass just overwrites the base class's .origin. The more I think about it, the more I think this isn't a big deal, and would be very hard to solve right now (I wrote the checkbox before I understood how FrameAttributes worked with the FrameMeta). Basically, to not re-add .origin for each subclass, we'd have to move the .origin._frame value onto the AstrometricFrame class and just override that. There doesn't seem to be an easy way to do that with FrameAttributes right now, as FrameAttributes don't know about their parent frame, so I think its not a problem worth solving. The metaclass can just add a new .origin frame each time.

I've left the .origin attribute on the base AstrometricFrame class so that it is there for documentation and tab-completion purposes.

TL;DR; Its not really a problem, the current implementation works and there isn't an easy way to do something better.

[taldcroft]

@eteq - I see your points about how this gets a little sticky given the transform-graph infrastructure. I'll stop quibbling now if you'll agree to lon and lat for attribute names (not dlon dlat). I still feel pretty strongly about that.

Is the plan then to have something like:

>>> af = AstrometricFrame(origin=SkyCoord(1, 2, unit='deg', frame=FK5))
>>> type(af)
AstrometricFrameFK5

[cdeil]

As a user, I don't care much about the class name. Even if you make it AstrometricFrameICRS and AstrometricFrameAltAz, it's still the same name for multiple different frames if I e.g. create two frames with different rotation here, or have AltAz frames with different pressure or location. So as a user I anyways have to look at the frame instance attributes not just the frame class names to understand what's going on. Now if having the origin frame class name be part of the astrometric frame class name helps e.g. with the transform graph machinery or is even necessary to make it work, then OK. I just wanted to mention that if a much simpler implementation is possible if the name is always AstrometricFrame, then I think it's OK for users.

Some more thoughts:

• 👍 to (lon, lat) as names (I think I said this before, not sure)
• Add a test with transformations between frames with different instance attributes (e.g. two or three AltAz frames, but with different center and rotation) to show that the transform machinery works in that case?
• Merge to master asap to give people at least a few days to test?
• Given that there has be very little user feedback on this new feature and the implementation is very complex, maybe add a disclaimer to the class docstring and astrometric docs section for the 1.2 release that this is not stable yet?

FYI: here's what I eventually want to do with this, but I haven't tried it yet:

• Compute AltAz astrometric coordinates for arrays of AltAz and Time values (ALTAZ telescopes slewing, pointing at a fixed RADEC). Do you think this "array of times" use case is possible with the current implementation?
• Given background models in AltAz FOV coordinates, reproject them into sky images in ICRS or Galactic coordinates. (Probably by transforming each pixel and then calling scipy.ndimage.map_coordinates?)

[astrofrog]

I'm +1 to lon/lat

[mhvk]

Also OK with lon/lat. Have not looked in enough detail to know how it looks now, but what the user might see much more than the actual class of the name is its repr; how does a SkyCoord of a star in an astrometry frame currently look like? (Sorry, have to run otherwise I'd fetch this and try it myself).

[eteq]

Oops, forgot to leave a comment here about my alexrudy#1 ... There I implemented lat/lon, and put the frame-specific names back in (following the compromise with @taldcroft ;). Also did some doc additions to clarify what's going on with those changes (in "note" form, so as not to distract the typical user), and some other minor tweaks.

I see @alexrudy merged it right away and the tests are passing. So I think if we merge this it closes #4931 now (unless all of a sudden a lot of opinions appear on changing the sign of the rotation).

@taldcroft - in the branch as it stands right now, it's almost what you said, but instead:

>>> af = AstrometricFrame(origin=SkyCoord(1, 2, unit='deg', frame=FK5))
>>> type(af)
astropy.coordinates.baseframe.AstrometricFK5

To better match all the other frames, which don't have "frame" in the class name.

And @mhvk, the __repr__s now look like this:

>>> af
<AstrometricFK5 Frame (rotation=0.0 deg, origin=<FK5 Coordinate (equinox=J2000.000): (ra, dec) in deg
    (1.0, 2.0)>)>
>>> SkyCoord(3*u.deg, 4*u.deg, frame=af)
<SkyCoord (AstrometricFK5: rotation=0.0 deg, origin=<FK5 Coordinate (equinox=J2000.000): (ra, dec) in deg
    (1.0, 2.0)>): (lon, lat) in deg
    (3.0, 4.0)>

[eteq]

Oh, and re: @cdeil's thoughts:

Add a test with transformations between frames with different instance attributes (e.g. two or three AltAz frames, but with different center and rotation) to show that the transform machinery works in that case?

That's a good idea... @alexrudy do you think you can whip this out? (Or @cdeil, maybe PR to @alexrudy's branch?)

Merge to master asap to give people at least a few days to test?

Agreed... Although someone has to do the point one up to do that ;)

Given that there has be very little user feedback on this new feature and the implementation is very complex, maybe add a disclaimer to the class docstring and astrometric docs section for the 1.2 release that this is not stable yet?

Definitely not in two places. Every time we've done that in two places we've forgotten to remove both at the same time... But It could quite reasonably go in the astrometric part of matchsep.rst? I guess I'm -0 to this because I don't think it's something that needs the warning - it's more likely to scare users away than anything. (That's in contrast to ecliptic where the problem was not enough feedback from relevant communities.) But if others think it's a good idea, I don't especially mind.

[alexrudy]

@eteq I'll write a test that goes between the same frame but with different attributes, and between two frames, to make sure those both work!

[eteq]

Awesome, thanks @alexrudy - once that's in, and a final decision is made on the note/warning in the docs, this should be good to go.

[mhvk]

I don't think a warning is needed.

Might it be an idea to adjust the repr to just give the name Astrometric since this is in fact how it is initialized? One could argue this is better both from the principle that one should be as close as possible to exec(repr(thing)) == thing) and since the FK5 part is obvious from the origin? @taldcroft: might this in fact address some of your concerns about not confusing users?

Separately, for the coordinates inside the frame when used inside SkyCoord, perhaps use str rather than repr?

p.s. None of the above is essential, obviously.

[alexrudy]

I've added tests for AstrometricAltAz to AstrometricAltAz with differing attributes. It required a tweak to the transform (hey, I guess that's why we write tests?).

I have not added a warning about instability to the docs anywhere, but I think I agree with @eteq and I'm sort of -0 on that one.

@mhvk I think changing the repr isn't trivial and might have to be a separate issue. Its buried in the repr of the representation.

[eteq]

@mhvk - mechanics aside (I agree with @alexrudy that it's potentially difficult), I disagree that hiding the (e.g.) FK5 part is good anyway. I think it is not just an "implementation detail" that the AstrometricFrame class is actually a class factory: it really is a different class in a sense that matters for the public API - they get injected into the transform graph, have different is behavior, etc. And while 95-99% of users aren't going to care one way or the other, the 1-5% that do (who I suspect are biased towards "likely contributors...") might be 100x more confused if they don't know that these are all distinct classes.

At any rate (aside from that point above, possibly?), I think this is now ready to go, as the tests are passing, and it has had quite a bit of review! Will give another day or so to see if any dissenting opinions (Esp. re: @cdeil's thought on the warning, although so far on balance the vote seems to be towards "not needed"), then merge.

[mhvk]

I'm fine with this, though my point about the repr really is that it is confusing to get a class name which you cannot import from anywhere, but need to create indirectly. But since one hardly looks at repr of the class anyway, it is something that may be best solved in SkyCoord.__repr__ rather than in the class, which makes it outside of the scope of this PR.

[eteq]

Alright, seeing no further changes asked for, I'm going to go ahead and merge this.

(@mhvk's last comment here I'm taking as a suggestion for a future PR - feel free to make an separate issue on that, @mhvk! Personally I think we want to leave it as-is, but we can discuss that in a separate space.)