search

asdf-format / asdf-coordinates-schemas

ASDF schemas for coordinates
https://asdf-coordinates-schemas.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
1 stars 3 forks source link

Add Geodetic Representations #40

Closed WilliamJamieson closed 1 year ago

WilliamJamieson commented 1 year ago

fixes #39

WilliamJamieson commented 1 year ago

resources/schemas/representation-1.1.0 .yaml has a space in the file name.

FYI it is more than just that. We need asdf-astropy support to complete the transition to the 1.1.0

WilliamJamieson commented 1 year ago

Looking into these geodetic classes it appears that the changes and API for this are not yet finalized (based off this open issue: astropy/astropy#11170).

The original PR adding these representations involved a discussion about deprecating EarthLocation.to_geodetic and making a new method that returns these new types: astropy/astropy#11086 However it was decided to leave these types as internal and keep the currently used astropy.coordinates.earth.GeodeticLocation that is produced when calling astropy.coordinates.earth.EarthLocation.to_geodetic.

It appears that the return type for this is unsupported in asdf-astropy on main and with this PR.

import asdf
import astropy.coordinates.earth

c = astropy.coordinates.earth.EarthLocation.of_site('Keck Observatory')
g = c.to_geodetic()
asdf.AsdfFile({'g': g})

fails with

Warning: Failed to serialize instance of <class 'astropy.coordinates.earth.GeodeticLocation'>, converting to list instead
  warnings.warn(f"Failed to serialize instance of {type(node)}, converting to list instead", AsdfWarning)

I'm inclined to wait until the new geodetic api is finished before adding support for it in the asdf schema.

I will close this PR since you feel this way.

I want to point out that these representations have been added to the public API for astropy: https://github.com/astropy/astropy/blob/08dba88089f124064492d3fde40919561640ea51/astropy/coordinates/representation/__init__.py#L9-L14 and that your API comments are irrelevant since we only care about the data contained within the object not its general API with astropy. Our support for astropy representations is based on the baseline object and the common way they store their information: https://github.com/astropy/asdf-astropy/blob/6c0e91a1db88d1c7253ac476dcc5eebc2a1a33fd/asdf_astropy/converters/coordinates/representation.py#L24-L39 not some other astropy API.