Add topocentric ITRS frame. Create direct transforms between it and o… - #13398

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 2022-07-27 19:50:52 UTC

I am concerned that the transform graph in the Read the Docs build under Astronomical Coordinate Systems Built-in Frame Classes doesn't show my new ITRS<->AltAz and ITRS<->HADec transforms. Am I missing something there?

I have been doing some more testing. My direct ITRS<->Observed transforms speed up transforms to AltAz from ITRS, TETE, and TEME by a factor of 4. TETE seems to automatically find the shortest path through the ITRS rather than through GCRS as before. For TEME, I used the example given in Working with Earth Satellites Using Astropy Coordinates in the documentation. That example shows a transformation from TEME to AltAz. The result is incorrect as it naturally assumes that the TEME coordinates include aberration as seen from the geocenter, which near Earth satellites do not. I found the following offsets after doing the transformation correctly as described in my updated documentation for the ITRS frame:

Az:   59.28803392 deg delta: 46.0031arcsec
El:   10.94799670 deg delta: 15.472arcsec
Dist: 1431.96640195 km delta: 4.723 m

I will update the example to show the correct way of doing this. BTW, it would be nice if the units label for arcsec included a space after the number.

Could someone please find the time to review this? @mhvk? @StuartLittlefair? I am hoping that it won't just languish for months or years.

Hi @mkbrewer - yes of course, sorry for leaving it to gather dust. I'll try and get to this early next week, but please remind me if I don't!

Hi @mkbrewer

Thanks for the careful replies. I'm definitely in favour of this PR, and I think some minor tweaks to the docs will help the end user. I've suggested these in the inline comments.

There are some minor code style errors that are preventing that CI test from passing. You need 2 blank lines around L292 and L954 in test_intermediate_transformations.py.

Finally, I think this is a change that would benefit from highlighting in the "What's New" section of the 5.2 release. Something along the lines of "A topocentric ITRS frame has been added that makes dealing with near-Earth objects easier" followed by a code example like:

    t = Time('J2010')
    obj = EarthLocation(-1*u.deg, 52*u.deg, height=10.*u.km)
    home = EarthLocation(-1*u.deg, 52*u.deg, height=0.*u.km)

    # Direction of object from GEOCENTER
    itrs_geo = obj.get_itrs(t).cartesian

    # now get the Geocentric ITRS position of observatory
    obsrepr = home.get_itrs(t).cartesian

    # topocentric ITRS position of a straight overhead object
    itrs_repr = itrs_geo - obsrepr

    # create a ITRS object that appears straight overhead for a TOPOCENTRIC OBSERVER
    topocentric_itrs_frame = ITRS(obstime=t, location=home)
    itrs_topo = topocentric_itrs_frame.realize_frame(itrs_repr)

    # convert to AltAz
    aa = itrs_topo.transform_to(AltAz(obstime=t, location=home))

One further question that I have: #13398 (comment)

One further question that I have: #13398 (comment)

Very good question! Yes - you should be concerned. I can't believe I missed this, but the transforms don't get registered with the graph unless the code in itrs_observed_transforms.py is executed, which happens at import time. You need to import this module in astropy/coordinates/builtin_frames/__init__.py for your new transforms to be registered!

Thanks!

Still no luck with the transform graph.

I don't know what it is about subtracting the two Cartesian positions in satellite.rst. This used to work, now it complains about differentials.

Well, I don't know about the graphic in the docs, but your transform is there in the frame transform graph. Previously this used to go through CIRS:

In [41]: from astropy.coordinates.builtin_frames import frame_transform_graph
    ...: from astropy.coordinates import AltAz, ITRS
    ...: 
    ...: trans = frame_transform_graph.get_transform(ITRS, AltAz)
    ...: for t in trans.transforms:
    ...:     print(f'{t.fromsys.name} -> {t.tosys.name}')
    ...: 
itrs -> altaz

For the line in satellites.rst, to be honest I don't know how this ever managed to work, but you need to replace L108 with

topo_itrs_repr = itrs_geo.cartesian.without_differentials() - siding_spring.get_itrs(t).cartesian

or, if you want to keep the satellite velocity (which you get using transform_to):

itrs_siding_spring = siding_spring.get_gcrs(t).transform_to(ITRS(obstime=t))   # CLUNKY, should add zero velocity to get_itrs
dp = itrs_geo.cartesian.without_differentials() - itrs_siding_spring.cartesian.without_differentials()
dv = itrs_geo.cartesian.differentials['s'] - itrs_siding_spring.cartesian.differentials['s']
topo_itrs_repr = dp.with_differentials(dv)

N.B - perhaps I'm overdoing this, but is it worth explaining in satellites.rst why you shouldn't just use itrs_geo.transform_to(AltAz(obstime=t, location=siding_spring))?

Oh yes. I somehow failed to notice the velocity in the TEME frame and didn't include that in my testing, which is why it worked. Velocity in the ITRS is independent of location, so I don't see the point of dv here. Why not just:

topo_itrs_repr = dp.with_differentials(itrs_geo.cartesian.differentials['s'])

Regarding the graphic. I found this in index.rst:

.. automodapi:: astropy.coordinates.builtin_frames
    :skip: make_transform_graph_docs
    :no-inheritance-diagram:

Does that mean building a new graph is being skipped?

I'm going to go with an example of carrying the velocity to the topocentric frame without dv.

Looks good now. The one failure is unrelated.

Unfortunately, the documentation build failed, but whatever caused that appears to be unrelated to this PR. And this after my transforms finally appeared in the transform graph after the last build.

Hmm... WARNING: The following HTTPError has occurred fetching https://matplotlib.org/stable//: 522 () -- I'll restart RTD build and see.

Is the coordinates failure in https://github.com/astropy/astropy/runs/7472108451?check_suite_focus=true related?

The matplotlib stuff looks unrelated.

No, it is not.

Yeah, ok. I see it in main too.

As far as CI is concerned, I think this is good, unless you also want "exotic" archs to run, then we can add "Extra CI" label; lemme know. Thanks!

Looks great @mkbrewer. Thanks for the effort; it cleans up dealing with near earth objects substantially. I'll hold off merging for a few days in case @mhvk wants to add anything.

You guys get my taskmaster of the year award! 😜

Failures are unrelated again.

@mkbrewer - OK, looks all OK to go in. Just squashed the commits a bit, to remove the PEP ones, etc. Will merge once the tests ran again. Thanks so much for a really nice contribution!

You are welcome! I hope that this is helpful for those who deal with the positions of satellites.