Consider changing astrometric frame rotation sign and names

@cdeil

#4909 is now in, adding the astrometric frames (or "field of view" frame, as @cdeil I believe knows it as).

Two things came up in discussion that amount to arbitrary choices:

The name of the coordinates for the frames. Currently it's "d<whatever the underlying frame's lon is called>" and "d<whatever the underlying frame's lat is called>", so e.g. AstrometricICRS is dra and ddec. But in various situations other names are used, like xi and eta for plate offsets from UCAC and POSS (see #mhvk's Finish up Astrometric frame and spherical_offsets_to #4909 (comment)). It's pretty trivial to change this, so we could do it anytime before the 1.2 release.
The sign of the final rotation of the frame. The current sense is left-handed, so that a positive rotation means for e.g. equatorial systems, put something that's at that position angle (E of N) so that it's pointing straight "up" (i.e., the positive latitude direction). But if it's judged that the other sign is better that's literally a one-character change to the code, so also easy to do any time before release.

My feeling is we should gather opinions through beta and then make a final call then. So discuss away if anyone has opinions (@mhvk, @cdeil, @smaret, @taldcroft, @Kevtron, @alexrudy, @r-owen?)

@mhvk

To continue some discussion from #4909 (comment) by @mhvk:

@eteq - on the names: from what I've seen from, e.g., POSS and UCAC calibration, if one goes for relative positions, sometimes xi and eta are used as local longitude and latitude relative to a plate centre (e.g., the UCAC paper by Zacharias et al. http://adsabs.harvard.edu/abs/2000AJ....120.2131Z). But those of course still have unknown position, so arguably that would be a bit different astrometric frame. Still, it may not be bad to use those for the names, since for large values, dra and ddec are not offsets in ra, dec any more (dra=12h, ddec=0 would get one to ra_ref+12h, -dec_ref). And it doesn't have problems at the poles...

I think dra ddec is still nominally reasonable if you interpret the deltas as "angle shift in the ra/dec direction" rather than just "ra1-ra0". But the name becomes a bit more confusing when you factor in the rotation, so I could see the argument for something else. Personally I really dislike xi and eta, though, because they are not very descriptive - I've had many conversations where people get them confused (unless you see it on a plot, of course). But I think more opinions are in order here (lets have further discussion in #4931)

As for the rotation, I think one always it to be such that with rot=0, a star to the North from the ref star would have a positive offset in latitude, with rot=90, that would hold for a star in the East, etc., i.e., the rotation defines where a positive offset in latitude goes, from N going through E.

I agree rot=0 definitely means just the straightforward case where something directly north -> +ive ddec and dra=0. But there's more question about the sign. I think you're advocating from swapping the sign from the current implementation, right? My thinking in the current implementation was the use case of "I've got an object at a PA (E of N) of blah in the underlying from, and want to align the N/S axis with that object".

@eteq

@eteq: On rotation, I think we want the same goal: if one has another star to the E of the reference star, then for rot=90, that is now in the direction of positive latitude. I think this is logical, but there may well be a standard that I'm unaware of...

And, yes, I dislike xi and eta too... I think overall I'd go for just lon and lat, but am curious to hear other suggestions.

What about dlon and dlat?

I prefer lon and lat as well. There is nothing in this coordinate system that is explicitly about delta values. It happens to have an usual origin and orientation, but otherwise just an everyday spherical coordinate system. I think that RA and Dec have a somewhat special meaning as being tied to particular strictly defined systems like ICRS, so lon and lat seem natural and avoids inventing new one-off attributes with no precedent.

@taldcroft

@taldcroft - There might be a misunderstanding here - you said:

I think that RA and Dec have a somewhat special meaning as being tied to particular strictly defined systems like ICRS

And I agree with that - it's not that every astrometric frame is dra/ddec, but that they're all d<something>. E.g., for galactic coordinates it's dl and db. I thought this was a better idea than just using e.g. ra and dec, because then it's not manifestly apparent that they're astrometric frames. Imagine, for example, an astrometric frame getting passed into a function, and then some novice contributor/user goes and looks and sees something like:

def show_coordinates(astrcoord):
    plt.scatter(astrcoord.ra, astrcoord.dec)

Just looking at that, you wouldn't understand why it gives you something that's not actual RA and Dec. By contrast, with

def show_coordinates(astrcoord):
    plt.scatter(astrcoord.dra, astrcoord.ddec)

it's immediately clear on inspection that this is some kind of offset (or at least somehow different from regular ra and dec).

But I could also see dlon/dlat - that's a bit more generic in meaning while still getting across that it's not just an "ordinary" frame.

@cdeil

Well I'm obviously way late in commenting, but I don't even understand what is meant by AstrometricGalactic or AstrometricAltAz. For instance Galactic is a very specific frame, so to attach the labels l and b (or even dl and db) to some other frame that is rotated and offset by some arbitrary amount doesn't make any sense to me. AltAz seems even weirder. Basically once you are transformed away from a standard frame then using the standard labels attached to those frames seems misleading / meaningless.

Going further down this path, I would have expected the API to be like:

my_frame = Astrometric(origin=SkyCoord(1, 2, unit='deg', frame='fk5'), rotation=20 * u.deg)
c_my_frame = SkyCoord(lon=3, lat=4, unit='deg', frame=my_frame)
c_my_frame.icrs
...

So basically the origin just provides a reference point on the sky (which might be internally converted to ICRS, but that doesn't concern the user), and this is followed by a rotation about that point. This entirely removes the whole concept of a particular input frame being permanently associated with an AstrometricFrame. BUT maybe I'm just not getting something about why AstrometricAltAz is meaningful. The original idea of a "field of view" frame per @cdeil sounds like it would not care about any standardized frame.

And from there I would say that lon / lat are reasonable generic choices. Again a particular AstrometricFrame is just like every other frame except that it is typically not a widely used one.

As far as I can see there are no built-in frames that use lon and lat as the default representation mapping, so seeing the following should alert the user that this coordinate object is slightly different:

def show_coordinates(astrcoord):
    plt.scatter(astrcoord.lon, astrcoord.lat)

@taldcroft

@taldcroft

Going further down this path, I would have expected the API to be like

That was indeed what we had originally wanted. But it turns out that has one problem: it requires that there be a defined transformation from Astrometric to every other frame on the transform graph, because the origin could be any frame type. And that creates all kinds of implementation problems (can go into that more if you want). But if you instead do this:

AstrometricFK5 = make_astrometric_cls(FK5)
my_frame =  AstrometricFK5(origin=SkyCoord(1, 2, unit='deg', frame='fk5'), rotation=20 * u.deg)
c_my_frame = SkyCoord(dra=3, ddec=4, unit='deg', frame=my_frame)
c_my_frame.icrs

then that works.

So basically the origin just provides a reference point on the sky

And there's the rub: it's not just that a reference point that's needed, an orientation is also required, otherwise "rotation" is meaningless (i.e. rotation with respect to what?). So AstrometricFK5(rotation=0) is oriented in the same way as FK5. And that was what I interpreted to be the key to what @cdeil wanted: a way to have a system that has the same AltAz orientation as the origin (at a particular time/atmospheric pressure/etc). So that's why each Astrometric frame has to be subtly different. And that's why I favor them having names derived from the base frame (i.e. dra/ddec): they are tied exactly to that frame if rotation=0, and that only changes for non-0 rotation.

I'm not sure if I'm being naive, but it seems like just a small change in your current machinery could allow the original API desire could be implemented. Basically:

Instead of encapsulating the origin-frame-specific class (e.g. FK5) creation in make_astrometric_cls, do that in an exposed Astrometric class that uses your existing AstrometricMeta to dynamically create the appropriate AstrometricFK5 class internally (or use the cached version).
Use the frame of origin to set the rotation frame when creating the class.
Change the class __name__ to Astrometric so it looks the same. I would argue this is not confusing because it is exactly the originally desired API and the object representation clearly expresses the origin coordinate values and frame, along with rotation.
In essence this is making a hardcoded backend class that does the work in a way that appears to the user to be dynamically done (with respect to handling the origin coordinates, origin frame orientation, and rotation).
Use some consistent attribute representation, lon, lat or perhaps dlon, dlat.

In [15]: my_frame.__class__.__name__ = 'Astrometric'

In [16]: my_frame
Out[16]: 
<Astrometric Frame (equinox=J2000.000, origin=<FK5 Coordinate (equinox=J2000.000): (ra, dec) in deg
    (1.0, 2.0)>, rotation=20.0 deg)>

I think this API is cleaner and less redundant (assuming it is technically feasible):

origin = SkyCoord(1, 2, unit='deg')

# 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)

And that's why I favor them having names derived from the base frame (i.e. dra/ddec): they are tied exactly to that frame if rotation=0, and that only changes for non-0 rotation.

This still confuses me, because even for rotation=0 a value of dra=10 arcsec and ddec=0 arcsec does not imply that ra and dec are doing the same thing. For origin=(0*u.deg, 89.999*u.deg), rotation=0 then a dra value corresponds to nearly a pure change in dec in the original frame. Calling it dra somehow implies a change in ra, but that can be entirely wrong. And of course once you allow a rotation then it is fully uncorrelated. So putting in new neutral names that don't tie back to the original frame names seems the only way to avoid being misleading.

So putting in new neutral names that don't tie back to the original frame names seems the only way to avoid being misleading.

+1 to always calling it lon, lat!

@taldcroft

I must admit I do like @taldcroft's API more, where the frame of the origin is used (and define the rotation); is this indeed possible?

@mhvk

@mhvk This is only possible with some really dark magic.

The coordinates transform graph uses classes to identify different frames. Since Astrometric would be a class, it would be registered as the transform between e.g. FK5 and Astrometric, and between AltAz and Astrometric, so to convert between an Astrometric frame with an AltAz origin to FK5, the coordinate transform graph would mistakenly try to convert directly (Astrometric -> FK5), instead of properly going via AltAz, by doing Astrometric -> AltAz -> FK5.

The dark magic here is that it is possible to use the subclasses as they are implemented now, if the __new__ method of Astrometric returns a subclass of Astrometric, not Astrometric itself, and registers that subclass in the transform graph during the call to __new__, but that feels like a fragile hack to me. I've done it in other python modules, but it doesn't seem like the right thing to do for Astropy.

Also, more generally, I think if we are letting rotation in, I vote for not using dra, ddec etc., because it gets really confusing with a position angle of 90º etc.

I've implemented a single sky offset with no rotation, then an "Instrument Frame" which can be rotated just like the instrument is rotated. In this case, the "Instrument Frame" uses x and y, as they are the axes names in the underlying instrument software, but I know that goes against the internal cartesian representation.

@alexrudy

.. registers that subclass in the transform graph during the call to new, but that feels like a fragile hack to me.

@alexrudy - that is what I was suggesting. I curious why you think that is fragile and am interested (both for this case and others) what might go wrong. I thought this is exactly what metaclasses are intended to do, and any use of a metaclass-based class is doing the magic of dynamically generating the class in advance of letting the user instantiate an object.

@taldcroft

@taldcroft I think this is one step beyond metaclasses. A metaclass adjusts the class before it is created. Once you have the class object, it is set, and is always that class object. In this case, Astrometric will just be the base class for AstrometricFK5, but Astrometric(origin...).__class__ is Astrometric will be broken. This might break code like isinstance(my_frame, type(my_other_frame)) or isinstance(my_frame, my_other_frame.__class__) in what you might consider a subtle way.

The hack-y feeling also comes up for me because I have to be a little careful with e.g. __new__ as it will be called twice in some cases.

That being said, if you don't think its hack-y, then I like the API. It just seems like we are fighting between the end-user API and understanding the transform graph.

@alexrudy

@alexrudy - thanks, that makes sense and it does make me slightly queasy that your examples would not work as expected.

I've been trying to wrap my head around whether there is a clean solution and I agree it is tricky. In the meantime, it seems that in this code and the inverse, the icrs is just a leftover and it should really be more like reference?

    @frame_transform_graph.transform(DynamicMatrixTransform, framecls, Astrometric)
    def icrs_to_astrometric(reference_frame, astrometric_frame):
        """Convert an ICRS coordinate to an Astrometric frame."""

        # Define rotation matricies along the position angle vector, and
        # relative to the origin.
        origin = astrometric_frame.origin.spherical
        mat1 = rotation_matrix(-astrometric_frame.rotation, 'x')
        mat2 = rotation_matrix(-origin.lat, 'y')
        mat3 = rotation_matrix(origin.lon, 'z')
        R = mat1 * mat2 * mat3
        return R

I did explicitly test by changing icrs to asdf and tests passed, so the icrs is not special. This is incredibly confusing when first looking at the code.

@taldcroft

@taldcroft I agree, we should remove the references to ICRS above.

@taldcroft

I've made a PR with @taldcroft's suggested API at #4941 so we can see exactly how hack-y this really is. Its not so bad, but it does result in some odd inconsistencies.

@alexrudy

@alexrudy - nice!

FYI - I started using the version in Astropy master this morning to check FOV coordinates against our proprietary HESS software here. There's currently a ~ 10 arcsec difference, I don't know why and only have time to continue debugging this tomorrow.
The FOV coordinates were only added very recently in the HESS software and haven't been debugged properly, so it's very well possible that the HESS results are off.

I looked at the astrometric tests in Astropy briefly and as far as I can see there isn't one that "proves" that the results are correct, e.g. pick some case where the correct answer is 90 deg or something that can be understood exactly without needing another code to check. Is that true or did I miss this test?

@cdeil

@cdeil - Doesn't the test_rotation test do what you want (here in current master) do what you want? Or am I misunderstanding what you're hoping to see?

@eteq

@eteq - Yes, test_rotation is exactly the kind of test I was thinking of. Sorry for the noise.

@mhvk

There's discussion in #4941 on the changes proposed there, but to summarize what I see so far on the specific elements in this issue:

the coordinate names should be changed to lat/lon (or maybe dlon/dlat? wasn't clear if others have clear opinions there)
no strong opinions voiced on the sign of the rotation, except maybe 👍 to the status quo by @mhvk (@mhvk, you might want to check out test_rotation) to confirm it's what you wanted, but I think it is?)

#4941 made some substantial changes triggered by this discussion, including a name change to lat/lon. The rotation sense was left as-is, as there seemed to be no clear disagreement there. But that addresses all the questions in this issue, so I'm closing. Thanks all, and as usual feel free to open up new issues if this is premature.

Navigation Menu

Repository navigation

Consider changing astrometric frame rotation sign and names #4931

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Consider changing astrometric frame rotation sign and names #4931

Description

Activity

eteq commented on May 16, 2016

mhvk commented on May 16, 2016

astrofrog commented on May 16, 2016

taldcroft commented on May 16, 2016

eteq commented on May 16, 2016

taldcroft commented on May 16, 2016

taldcroft commented on May 16, 2016

eteq commented on May 17, 2016

taldcroft commented on May 17, 2016

taldcroft commented on May 17, 2016

cdeil commented on May 17, 2016

mhvk commented on May 17, 2016

alexrudy commented on May 17, 2016

alexrudy commented on May 17, 2016

taldcroft commented on May 17, 2016

alexrudy commented on May 17, 2016

taldcroft commented on May 17, 2016

alexrudy commented on May 17, 2016

alexrudy commented on May 17, 2016

taldcroft commented on May 17, 2016

cdeil commented on May 18, 2016

eteq commented on May 18, 2016

cdeil commented on May 19, 2016

eteq commented on May 24, 2016

eteq commented on May 27, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions