Repository navigation
Consider making EarthLocation an ITRS subclass #4261
Description
Activity
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 fromITRS. 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 ofITRSbecauseITRShas 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_itrsfunction 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 likeloc.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
EarthLocationwould be anITRSposition 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:
-
- Internally,
EarthLocationcould useCartesianRepresentationinstead of be based on aQuantityrecord array.
- Internally,
-
- The geodetic representations could be implemented using variants of
SphericalRepresentation(variants, since there'sheightover a reference ellipsoid instead ofradius).
- The geodetic representations could be implemented using variants of
-
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
BaseCoordinateFramewe can just slap on a bit of syntactic sugar to reproduce thegeodeticandgeocentricinterfaces 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
EllipticalRepresentationor something like that for the geodetic side - hopefully that could be mostly copy-and-pasted from the existing code intorepresentation.pyJust as a placeholder: in #5486, trying to serialize astropy objects, it became obvious that two other small things are presently missing: a proper
infoclass (not that inherited fromQuantity) that describes the basic attributes that are needed (so,x,y,z, presently), and the ability to set the ellipsoid as part of thefrom_geocentricclass method.While reviewing astropy-learn/astropy-tutorials#460, I started reading
EarthLocationdocs more closely, and were surprised that it's aQuantitysubclass. I tried a few commonQuantitymethods on anEarthLocationobject 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
EarthLocationshould not have a base class, and instead contain the coordinate data as an instance property of typeCartesianRepresentation?@astrojuanlu - I'm still struggling with this. In a way, I think
EarthLocationis trying to do too many things in one class. Right now, it subclassesQuantity, 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 betweenCartesianRepresentationand a newGeodeticRepresentation, i.e., one would doel.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
EarthLocationitself 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 ofget_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 anlocationattribute to store the observer's location, whileGCRSneedsobsgeoloc,obsgeovel(see #10996).Anyway, overall I think the time is ripe to think how to improve this! What do you think would work best?
The idea of making
EarthLocationa subclass ofITRSlooks good at first sight. However, after reflecting a bit whether the relationship should beIS-AorHAS-A(as mentioned in the "Composition over Inheritance" Wikipedia page), I noticed a few weird things about the inheritance proposal:- If
EarthLocationis-aITRSand therefore is-aBaseCoordinateFrame, do we expectcoord.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 expectloc.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 ofEarthLocationextra data, it has the same problem asinfo. If we create a newGeodeticRepresentation, should it be stored there? However, in words of @eteq when I was attempting to create aClassicalElementsRepresentation(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
EarthLocationshould stop being aQuantitysubclass, 99 % sold on creating{WGS84,GRS80,WGS72}GeodeticRepresentation(to avoid a genericGeodeticRepresentationwithout a good way to store theellipsoid), 80 % sure thatcoord.transform_to(EarthLocation(...))shouldn't be a thing, and 70 % sure thatEarthLocationshould not be aITRSsubclass, but rather contain the data as a property, along with the metadata (at the cost of losingloc.transform_to(GCRS(...))syntactic sugar).- If
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
EarthLocationfor now its own class which holds a representation, which can beCartesianor any of the threeGeodeticRepresentationsas needed.But it is probably worth looking at bit more at #10996... In particular, in sunpy, they deal with locations via an
observerattribute which is simply a frame instance. That would solve the disconnect between what is needed forAltAzandGCRS.Reacted by Juan Luis Cano RodríguezAfter reading #10996 and the recent changes made to TEME and CIRS, I'm wondering... If an "observer/location" could be described by an
ITRSinstance (or any other frame, for that matter), and if we had proper*GeodeticRepresentation, wouldn't that makeEarthLocationalmost pointless, its only use being the.infometadata?@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
SkyCoordthe 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).
Reacted by Juan Luis Cano RodríguezDraft of geodetic representations: #11086
See #4254 (comment)
Right now, it is a
Quantitysubclass, which may not make most sense. It doesn't particularly needobstimethough.