Implement framework for observed hour angle-declination transforms. - #11676

Hello @mkbrewer 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-05-20 23:41:13 UTC

I just want to say that I will get round to reviewing this ASAP, but I am under a lot of work pressure until May 15th, so probably not before then.

Regarding tests for accuracy, my point is that these would be redundant. For instance, if test_aa_high_precision() passes, then it can be guaranteed that the corresponding test for HADec would also pass, should I create it. This is because both AltAz and HADec are derived from the same vector in atioq().

@mkbrewer - OK, I rebased to get rid of the merge conflicts and made the simplification I suggested. I also took the liberty to squash the multiple smaller commits. Locally, the tests pass, and on second thought I agree with you that there is not much point in separate precision tests.

What still would be good, I think, is to have at least a simple example somewhere in the documentation, like

In [1]: from astropy import units as u

In [2]: from astropy.coordinates import EarthLocation, SkyCoord, HADec

In [3]: from astropy.time import Time

In [4]: el = EarthLocation.of_site('keck')

In [5]: t = Time('2010-03-21T10:00:00')

In [6]: sc = SkyCoord(12*u.hourangle, 10*u.deg)

In [7]: hadec = HADec(obstime=t, location=el)

In [8]: sc.transform_to(hadec)
Out[8]: 
<SkyCoord (HADec: obstime=2010-03-21T10:00:00.000, location=(-5464487.81759887, -2492806.59108569, 2151240.19451846) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0.0, obswl=1.0 micron): (ha, dec) in deg
    (353.19976268, 9.93902734)>

(where I verified the result with the veritable skycalc)

Regarding documentation, can you handle that? I can't find any similar example for either AltAz or TETE anywhere in the docs directory, so I wouldn't know where to start.

@mkbrewer - hmm, you're right, it is actually far from obvious where a simple example about how to use HADec (or AltAz for that matter) would go in the documentation. I do see that the list of frames available in, e.g., https://docs.astropy.org/en/latest/coordinates/skycoord.html is quite out of date. So, let me (1) cc @eteq, @adrn for possible suggestions; and (2) suggest that perhaps we should leave this to a separate issue that is maybe more specifically about the various frames and their uses (which I'll raise if the others agree).

I think otherwise this PR is in very good shape, though ideally @StuartLittlefair has a look as well (@StuartLittlefair - the addition is actually as simple as @mkbrewer had promised it would be).

@StuartLittlefair - would you have some time to review this HADec implementation?

OK. I've added a test. I'm likely to be tangled up in units though. Does ha need to be converted to u.hourangle somehow? Will allclose() work with the tolerance set to u.uas? Unfortunately, I have no way of testing this myself.

@mkbrewer - yes, with units those things should "Just work"! The changes look good, and I'll get this in once the tests pass. Thanks again!

Sadly the new test fails...

OK, it is just a different wrapping of ha. I guess that brings up the question of whether HA should be between -12 and 12 hours, or between 0 and 24...

OK, I pushed a fix, and also added a test that does not rely on remote data - using the "overhead" test so that the answers are known to be right by inspection. Will merge if tests now pass.

@mhvk Hour angle always ranges from -12 to +12.

Yes, I felt that made more sense, but that is not what your HADec class does.... I'll push an update.

That's odd. Both atioq() and ae2hd() return the result of atan2(). This should always lie between -pi and pi.

@mkbrewer - the problem is that the [Unit]SphericalRepresentation class uses a Longitude for which the default wrap angle is 360 degrees. I now fixed it following what is done in SkyOffsetFrame - but I'll raise another issue that we should make this more flexible - see #11767.

Anyway, I think the current fix is the best we can do without a more general solution... And since tests pass, I'll merge it. Thanks again!