Consider making EarthLocation an ITRS subclass

See #4254 (comment)
Right now, it is a Quantity subclass, which may not make most sense. It doesn't particularly need obstime though.

To clarify just a bit more on this: it's actually a feature that it doesn't have an obstime - that's the main thing that makes it different from ITRS. There's value in having an object that is a latitude/longitude/altitude but not necessarily at a particular time (which ITRS does have). So conceptually I don't think it can be a subclass of ITRS because ITRS has to fit into the transformation graph, and this cannot (it's degenerate how to transform to any other frame if you don't specify a time).

But what might make sense is to have it be a new frame class on its own, not hooked into the frame transform graph at all. You'd have to use the get_itrs function as it exists right now to actually get something that's transformable, but that's probably good because it forces the user to think about the time.

Yet another option is to have it be hooked into the frame transform graph via ITRS. Then users could just do standard things like loc.transform_to(ITRS(obstime=mytime)). The problem is that then they can also do things like loc.transform_to(AltAz(obstime='J2010',...)), and you might naturally think that this is the same as asking for the EarthLocation in J2010... but in fact, subtly, you actually get J2000 (the default for ITRS). So that's why I like the above approach of requiring the user to explicitly convert to an ITRS at a particular time - that way they are forced to think about what they actually want.

I don't know that it is a feature to discount that continental plates move... But of course it is true that for many applications we simply do not care.

It seems that in the end EarthLocation would be an ITRS position that has zero velocity. But maybe that is best done when we have support for velocities (and proper motions!).

In the meantime, there are some small steps that could go towards unifying the classes:

1. Internally, EarthLocation could use CartesianRepresentation instead of be based on a Quantity record array.
1. The geodetic representations could be implemented using variants of SphericalRepresentation (variants, since there's height over a reference ellipsoid instead of radius).

Yeah, that all sounds good. I guess what I was thinking above is that making it a regular frame object will just be a convenient way to do that - if it inherits from BaseCoordinateFrame we can just slap on a bit of syntactic sugar to reproduce the geodetic and geocentric interfaces and then we can use all the other frame-related machinery for string formatting and keeping track of names and such.

And I like the idea of an EllipticalRepresentation or something like that for the geodetic side - hopefully that could be mostly copy-and-pasted from the existing code into representation.py

Just as a placeholder: in #5486, trying to serialize astropy objects, it became obvious that two other small things are presently missing: a proper info class (not that inherited from Quantity) that describes the basic attributes that are needed (so, x, y, z, presently), and the ability to set the ellipsoid as part of the from_geocentric class method.

@mhvk

While reviewing astropy-learn/astropy-tutorials#460, I started reading EarthLocation docs more closely, and were surprised that it's a Quantity subclass. I tried a few common Quantity methods on an EarthLocation object and a few didn't work:

>>> vla = EarthLocation.of_site("vla")
>>> vla
<EarthLocation (-1601184.40191992, -5041989.95569235, 3554875.07685646) m>
>>> vla.to(u.km)  # OK
<EarthLocation (-1601.18440192, -5041.98995569, 3554.87507686) km>
>>> vla.to_value(u.km)  # Fail!
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/juanlu/Personal/Astropy/astropy/astropy/units/quantity.py", line 746, in to_value
    value = value * scale
TypeError: invalid type promotion
>>> vla + [100, 100, 100] * u.km
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/juanlu/Personal/Astropy/astropy/astropy/units/quantity.py", line 477, in __array_ufunc__
    result = super().__array_ufunc__(function, method, *arrays, **kwargs)
TypeError: invalid type promotion
>>> [100, 100, 100] * u.km + vla
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/juanlu/Personal/Astropy/astropy/astropy/units/quantity.py", line 474, in __array_ufunc__
    arrays.append(converter(input_) if converter else input_)
  File "/home/juanlu/Personal/Astropy/astropy/astropy/units/quantity_helper/helpers.py", line 41, in <lambda>
    return lambda val: scale * val
TypeError: invalid type promotion

@mhvk @eteq What's your current position on this issue, in case it changed since your latest comments here years ago? In particular:

Internally, EarthLocation could use CartesianRepresentation instead of be based on a Quantity record array.

Do you mean that EarthLocation should not have a base class, and instead contain the coordinate data as an instance property of type CartesianRepresentation?

@astrojuanlu

@astrojuanlu - I'm still struggling with this. In a way, I think EarthLocation is trying to do too many things in one class. Right now, it subclasses Quantity, but with a structured dtype (which is why most methods do not work), and it mixes calculations on the representations with transformations. I think more logically these should be separate, so that indeed the coordinate data would become an internal attribue. E.g., the changes between geocentric and geodetic could then be dealt with as transformations between CartesianRepresentation and a new GeodeticRepresentation, i.e., one would do el.represent_as('geodetic'). That would also have the advantage that methods like .transform, .cross, etc., become available (and one could actually add a vector...).

In such a revised model EarthLocation itself probably would become more like an ITRS coordinate frame (or even like a SkyCoord, if the frame can change), which contains the representation within it, and has methods to transform to other frames (i.e., instead of get_gcrs() one would just to .transform_to(GCRS(...))). This may also help with how it is used inside coordinates transformations, where right now it is rather a pain that some frames want to have an location attribute to store the observer's location, while GCRS needs obsgeoloc, obsgeovel (see #10996).

Anyway, overall I think the time is ripe to think how to improve this! What do you think would work best?

@eteq

The idea of making EarthLocation a subclass of ITRS looks good at first sight. However, after reflecting a bit whether the relationship should be IS-A or HAS-A (as mentioned in the "Composition over Inheritance" Wikipedia page), I noticed a few weird things about the inheritance proposal:

If EarthLocation is-a ITRS and therefore is-a BaseCoordinateFrame, do we expect coord.transform_to(EarthLocation(...)) to work?
What to do with info, which at the moment is used to store metadata like the site name? Do we expect loc.transform_to(GCRS(...)) to retain such metadata in any way?
Combination of the two points above: do we expect the roundtrip transformation to return the exact same object? loc.transform_to(GCRS(...)).transform_to(EarthLocation(...))?
Where should we store the ellipsoid? If it's part of EarthLocation extra data, it has the same problem as info. If we create a new GeodeticRepresentation, should it be stored there? However, in words of @eteq when I was attempting to create a ClassicalElementsRepresentation (think sma, ecc, inc, raan, argp, nu), "Stateful information like that isn’t really supported by representations because they are meant to be “stateless” aside from the actual locations themselves" (poliastro issue about this)

In sum, I fear I have more questions than answers, but I am 100 % convinced that EarthLocation should stop being a Quantity subclass, 99 % sold on creating {WGS84,GRS80,WGS72}GeodeticRepresentation (to avoid a generic GeodeticRepresentation without a good way to store the ellipsoid), 80 % sure that coord.transform_to(EarthLocation(...)) shouldn't be a thing, and 70 % sure that EarthLocation should not be a ITRS subclass, but rather contain the data as a property, along with the metadata (at the cost of losing loc.transform_to(GCRS(...)) syntactic sugar).

I think I agree with most (perhaps just slightly different percentages!). My guess is that the higher percentage parts are also the easier to address without breaking backward compatibility, i.e., make EarthLocation for now its own class which holds a representation, which can be Cartesian or any of the three GeodeticRepresentations as needed.

But it is probably worth looking at bit more at #10996... In particular, in sunpy, they deal with locations via an observer attribute which is simply a frame instance. That would solve the disconnect between what is needed for AltAz and GCRS.

After reading #10996 and the recent changes made to TEME and CIRS, I'm wondering... If an "observer/location" could be described by an ITRS instance (or any other frame, for that matter), and if we had proper *GeodeticRepresentation, wouldn't that make EarthLocation almost pointless, its only use being the .info metadata?

@astrojuanlu

@astrojuanlu - indeed, in a way that would be the point, to make it pointless! I guess the main use would be to provide an interface to a coordinate for which the basic assumption is that it is fixed to the Earth (like for SkyCoord the basic assumption is it is fixed to the sky).

But I think the nice part is that the problem is quite separable - one could start by just having the different representations before moving on to making it a frame (or something more easily transferable to a frame).

Draft of geodetic representations: #11086

Navigation Menu

Repository navigation

Consider making EarthLocation an ITRS subclass #4261

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Consider making EarthLocation an ITRS subclass #4261

Description

Activity

eteq commented on Oct 22, 2015

mhvk commented on Oct 22, 2015

eteq commented on Oct 23, 2015

mhvk commented on Dec 5, 2016

astrojuanlu commented on Nov 22, 2020

mhvk commented on Nov 22, 2020

astrojuanlu commented on Nov 22, 2020

mhvk commented on Nov 22, 2020

astrojuanlu commented on Nov 23, 2020

mhvk commented on Nov 23, 2020

astrojuanlu commented on Nov 25, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions