Add geodetic representations by astrojuanlu · Pull Request #11086 · astropy/astropy

astrojuanlu · 2020-11-25T11:03:01Z

Description

In line with discussions at #4261, this pull request aims to add geodetic representations.

I started with WGS84GeodeticRepresentation to touch base and get a few eyes on my approach. In particular, I don't think we can cleanly have a generic GeodeticRepresentation where the ellipsoid is an extra parameter, retained in roundtrip transformations, but I would be glad to be proven wrong.

The tests are almost a copy-paste of TestSphericalRepresentation, with a few changes:

height instead of distance
height can only be a Quantity, not a Distance
I didn't add differentials (not sure if worth it)

Besides, I also adapted test_gc2gd and test_gd2gc from test_earth.py to make sure we get the same results.

mhvk

@astrojuanlu - this looks good! I'm actually pleasantly surprised to see how relatively simple it is to add - the work is mostly in the tests!

Beyond the small in-line comments: I think these representations belong in earth.py as they are not useful as general representations. Similarly, I'd put the tests in test_earth. (Or perhaps a new earth_representations or even an earth/ module. Though seems a bit overkill right now.)

I agree that for now we can ignore velocities - we can add the ability to mimic observations from airplanes when the need arises!

How were you thinking of proceeding? A first step might simply be to remove the transformation machinery in EarthLocation and instead go through these representations inside from_geodetic and lon, lat, heigh - to_geodetic() may need some kind of deprecation warning as right now it returns a namedtuple and people may count on it. (I think that is OK, we should move to .represent_as anyway, making it more frame-like.)

p.s. I'll change this to draft right now just to be sure we don't accidentally merge.

astrojuanlu · 2020-11-26T20:31:18Z

Thanks for the prompt review @mhvk !

I agree with moving this to earth.py and with all your comments. I wanted to think about the way forward with EarthLocation today but I couldn't, so I don't have any ideas yet - will give it some thought tomorrow.

mkbrewer · 2020-11-27T11:14:41Z

I agree that for now we can ignore velocities - we can add the ability to mimic observations from airplanes when the need arises!

Don't forget balloons! https://askdrphilastro.wordpress.com/2019/12/15/blast-antarctica-2019-2020/

mhvk · 2020-11-27T13:07:46Z

Don't forget balloons!

How can I forget my own colleague! So, we will have to get there...

astrojuanlu · 2020-11-30T10:26:00Z

Addressed the first batch of comments, going to look into EarthLocation now.

astrojuanlu · 2020-11-30T12:40:21Z

What I did in the end was to deprecate .to_geodetic and .to_geocentric without changing their return values, and added a new .represent_as.

This is ready for a second pass!

astrojuanlu · 2020-11-30T14:12:42Z

Many test failures due to the AstropyDeprecationWarning I introduced - I guess no internal calls to deprecated methods are allowed. Will fix it.

mkbrewer · 2020-11-30T21:02:11Z

I would just note that many locations on Earth have heights that are below that of the ellipsoid. I seems a bit unreasonable to force a different method for creating an EarthLocation in these instances.

mhvk · 2020-11-30T21:25:52Z

Darn, we'd need allow_negative. I think for now it would be best to just proceed with a Quantity - we can deal with insisting it is a length differently/later.

mhvk

@astrojuanlu - this is starting to look very nice. Bit of a mix of larger and probably overly detailed comments.

More general one is where we stop. One option is actually a bit before where you are now, i.e., not add deprecation messages yet, but just introduce the new representations and use them internally only for the transformations. This has the advantage that no tests will fail yet. Arguably, in that case, we would also not yet add the represent_as method (which is not tested yet, so that is fine, I think).

The other is perhaps a bit further than you are now, whether EarthLocation stops becoming a Quantity and instead stores the representation in .data like a frame (or perhaps as an intermediate suggestion still shares the data with a CartesianRepresentation).

But my sense would be to take the smaller step suggested above, so we can get this in and follow-up on it.

mhvk · 2020-11-30T22:15:13Z

        For the conversion to geodetic coordinates, the ERFA routine
        ``gc2gd`` is used.  See https://github.com/liberfa/erfa
        """
+        warn("Method .to_geodetic(ellipsoid) is deprecated, "


Agree with the deprecation message, but in analogy with a frame or sky coord, I think there is little against still having the properties for particular representations, i.e., I think we should allow (and also refer to in the warning message),

el.{wgs84,...}geodetic el.cartesian

Removed the deprecation message for now, but saving this for the future.

mhvk · 2020-11-30T22:18:29Z

+            llh.lat,
+            u.Quantity(llh.height, self.unit, copy=False))
+
+    def represent_as(self, other_class):


Again seeing EarthLocation as more like a frame/skycoord rather than a representation, I think we can allow strings. I'd suggest following the nomenclature in their represent_as method and calling it base.

Removed represent_as for now, but saving this for the future.

mhvk · 2020-11-30T22:25:16Z

        return new_array.view(self.dtype).reshape(self.shape)
+
+
+class BaseGeodeticRepresentation(BaseRepresentation):


I think this will get registered in REPRESENTATION_CLASSES, which is not ideal... Pinging @adrn for ideas; should we allow a name = False explicit way to avoid registration? (Note: this should not have to hold up this PR!)

mhvk · 2020-11-30T22:32:19Z

    assert str(greenwich.info).startswith('name = Royal Observatory Greenwich')
+
+
+class TestWGS84GeodeticRepresentationRepresentation:


I would shrink this class substantially - many of the tests are done on Spherical to just have them done once, no need to do them again. Also, do re-align for the longer name...

Thinking more, since all new tests are (very nicely) just focussed on the representations, how about a new test_geodetic_representations?

I moved these tests to test_geodetic_representations.py, but I'm not sure which ones to remove.

astrojuanlu · 2020-12-01T08:27:50Z

But my sense would be to take the smaller step suggested above, so we can get this in and follow-up on it.

I agree - I will remove the deprecation warnings and remove represent_as for now, and follow up in a future PR. Addressing your comments now.

mkbrewer · 2020-12-02T13:24:29Z

While supporting alternate ellipsoids is fine, you should realize that there is more work involved in doing so. As currently written, ErfaAstrom.apco() calls erfa.apco() which calculates the observer's position and velocity based on the WGS-84 ellipsoid. Thus geodetic coordinates based on an alternate ellipsoid will yield incorrect results in any ICRS<->CIRS or ICRS<->AltAz transform.

mkbrewer · 2020-12-02T13:33:05Z

I should note that this relates to task 1) in issue #11072

astrojuanlu · 2020-12-06T18:17:35Z

Warnings are legitimate:

/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/11086/lib/python3.8/site-packages/astropy/coordinates/earth.py:docstring of astropy.coordinates.GRS80GeodeticRepresentation:1: WARNING: py:class reference target not found: astropy.coordinates.earth.BaseGeodeticRepresentation
writing output... [ 16%] api/astropy.coordinates.SphericalCosLatDifferential .. api/astropy.io.ascii.Latex
/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/11086/lib/python3.8/site-packages/astropy/coordinates/earth.py:docstring of astropy.coordinates.WGS72GeodeticRepresentation:1: WARNING: py:class reference target not found: astropy.coordinates.earth.BaseGeodeticRepresentation
/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/11086/lib/python3.8/site-packages/astropy/coordinates/earth.py:docstring of astropy.coordinates.WGS84GeodeticRepresentation:1: WARNING: py:class reference target not found: astropy.coordinates.earth.BaseGeodeticRepresentation
...
[changelog_links] Adding changelog links to "changelog"
<rst_epilog>:14: WARNING: py:class reference target not found: astropy.coordinates.earth.BaseGeodeticRepresentation

fixing

See astropygh-11104.

Also ensure longitude wraps at 180.

Turning it from a tuple into a dict.

mhvk · 2020-12-29T00:14:34Z

@astrojuanlu - OK, I also changed the tests now. Plus one thing that you should sign off on: to make ELLIPSOIDS a dict with the actual representations as values. I think it works quite nicely, mostly because #11090 already replaced the meta class with __init_subclass__, so one can now easily chain things. See 4612ca1.

mhvk

Approving, but, as noted, @astrojuanlu, would be best if you approve the changes I made!

astrojuanlu · 2020-12-29T10:34:45Z

                    'lat': Latitude,
                    'height': u.Quantity}

+    def __init_subclass__(cls, **kwargs):


Nice trick!

astrojuanlu

Can't approve my own pull request. This looks good to me, thanks @mhvk for the final tweaks!

astrojuanlu requested review from eteq and mhvk November 25, 2020 11:03

github-actions Bot added the coordinates label Nov 25, 2020

astrojuanlu mentioned this pull request Nov 25, 2020

Consider making EarthLocation an ITRS subclass #4261

Open

mhvk reviewed Nov 25, 2020

View reviewed changes

Comment thread astropy/coordinates/representation.py Outdated

Comment thread astropy/coordinates/representation.py Outdated

Comment thread astropy/coordinates/representation.py Outdated

Comment thread astropy/coordinates/representation.py Outdated

mhvk marked this pull request as draft November 25, 2020 15:07

mhvk added Refactoring Enhancement labels Nov 25, 2020

astrojuanlu force-pushed the geodetic-representations branch from 6b09266 to b227ed9 Compare November 30, 2020 10:18

astrojuanlu added this to the v4.3 milestone Nov 30, 2020

astrojuanlu force-pushed the geodetic-representations branch from b227ed9 to e831960 Compare November 30, 2020 10:23

astrojuanlu commented Nov 30, 2020

View reviewed changes

Comment thread astropy/coordinates/tests/test_earth.py Outdated

astrojuanlu requested a review from mhvk November 30, 2020 12:40

mhvk reviewed Nov 30, 2020

View reviewed changes

astrojuanlu force-pushed the geodetic-representations branch from 0a95f85 to 988b8fe Compare December 1, 2020 09:27

astrojuanlu marked this pull request as ready for review December 1, 2020 09:30

mhvk mentioned this pull request Dec 4, 2020

Add reference frames of planets and other bodies #5898

Open

astrojuanlu force-pushed the geodetic-representations branch from 221d454 to 900fbfa Compare December 6, 2020 17:51

astrojuanlu and others added 16 commits December 28, 2020 17:16

Avoid copy creation

c5dda3b

Parametrize ellipsoid

9c953a4

Move geodetic representations to earth.py

bb9d183

Style

5c242db

Skip geodetic differentials for now

ae10f46

Set a default height

3632e71

Enforce length units, disallow negative heights

2fa45df

WIP: Temporarily remove height validation

c01a2a0

See astropygh-11104.

Reuse representation transformations in EarthLocation

4d47917

Docstring improvements

fed447f

Remove duplicated code in tests

8b03381

Move tests for geodetic representation to separate file

abfb841

Export new geodetic coordinates

f05846b

Add changelog entry

510cc8c

Expose BaseGeodeticRepresentation as well

4a86c8a

Support units for erfa_ufunc.gc2gd and gd2gc

1d64541

mhvk force-pushed the geodetic-representations branch from 783303e to 8226e48 Compare December 29, 2020 00:01

mhvk added 4 commits December 28, 2020 19:08

Simplify conversion, avoid Distance, ensure angles are in degrees.

1d95f07

Also ensure longitude wraps at 180.

Add a few extra tests.

081b68c

Elaborate a bit more in changelog.

b5a3733

Auto-fill ELLIPSOIDS by the geodetic classes.

4612ca1

Turning it from a tuple into a dict.

mhvk force-pushed the geodetic-representations branch from 8226e48 to 4612ca1 Compare December 29, 2020 00:09

mhvk approved these changes Dec 29, 2020

View reviewed changes

astrojuanlu commented Dec 29, 2020

View reviewed changes

astrojuanlu merged commit 9cefcb9 into astropy:master Dec 29, 2020

astrojuanlu deleted the geodetic-representations branch December 29, 2020 10:40

mhvk mentioned this pull request Apr 30, 2023

Refactor EarthLocation as instance of BodyLocation. Add flattening and equatorial radius as parameters. #14714

Closed

braingram mentioned this pull request May 23, 2023

Add Geodetic Representations asdf-format/asdf-coordinates-schemas#40

Closed

		return new_array.view(self.dtype).reshape(self.shape)


		class BaseGeodeticRepresentation(BaseRepresentation):

		assert str(greenwich.info).startswith('name = Royal Observatory Greenwich')


		class TestWGS84GeodeticRepresentationRepresentation:

Conversation

astrojuanlu commented Nov 25, 2020

Description

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astrojuanlu commented Nov 26, 2020

Uh oh!

mkbrewer commented Nov 27, 2020

Uh oh!

mhvk commented Nov 27, 2020

Uh oh!

Uh oh!

astrojuanlu commented Nov 30, 2020

Uh oh!

astrojuanlu commented Nov 30, 2020

Uh oh!

astrojuanlu commented Nov 30, 2020

Uh oh!

mkbrewer commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Nov 30, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

astrojuanlu commented Dec 1, 2020

Uh oh!

mkbrewer commented Dec 2, 2020

Uh oh!

mkbrewer commented Dec 2, 2020

Uh oh!

astrojuanlu commented Dec 6, 2020

Uh oh!

mhvk commented Dec 29, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrojuanlu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkbrewer commented Nov 30, 2020 •

edited

Loading