APE26: Removing data storage (representations) from coordinate frames

[jeffjennings]

Coordinate frames in astropy.coordinates currently store metadata used to construct the frame (i.e. for transforming between frames) and may also store coordinate data itself. This duplicates functionality with SkyCoord, which acts as a container for both coordinate data and reference frame information. We propose to change the frame classes such that they only store metadata and never coordinate data. This would make the implementation more modular, remove ambiguity for users from having nearly duplicate functionality with slightly different APIs, and better satisfy the principle of Separation of Concerns.

Authors of this APE (alphabetical): Jeff Jennings @jeffjennings, Adrian Price-Whelan @adrn, Nathaniel Starkman @nstarman, Marten van Kerkwijk @mhvk

[adrn]

Pinging @eerovaher @eteq @StuartLittlefair @Cadair @ayshih @taldcroft as other "stakeholders" who may want to take a look at this and review!

[eerovaher]

I am convinced that there should be exactly one type for coordinate data. This would prevent users from getting confused about which type they should be using and simplifies things for both astropy and downstream developers because we'll only have to support one type of coordinate data as input for our functions.

This leaves two possibilities: the single type is a class, which is what this APE proposes, or the single type is a Protocol, which is what Implement BaseCoordinateFrame.frame property (astropy/astropy#16356) (that the APE mentions) proposed. Implementing a Protocol would be backwards-compatible and wouldn't cause disruption for downstream packages. In every other regard this APE is a better idea.

[eerovaher]

Not just code. It's even worse in tests because we have to test both SkyCoord and BaseCoordinateFrame methods with both SkyCoord and BaseCoordinateFrame inputs. Not testing all the combinations has led to bugs going unnoticed, e.g. astropy/astropy#15659

[eerovaher]

This is relevant not only for documentation but also for type annotations. Looking up BaseCoordinateFrame methods via SkyCoord.__getattr__() is too dynamic for static type checkers. It is possible to partially alleviate this by writing out stubs in SkyCoord (e.g. https://github.com/astropy/astropy/blob/24d48a9e2ef90af5383bf76e6d098e6d193d8186/astropy/coordinates/sky_coordinate.py#L178-L181), but type checkers cannot ensure that the stubs agree with the signatures of the actual methods.

[eerovaher]

I find it confusing that you have not used the pull request titles.

[jeffjennings]

Good point, thank you. Fixed in 46f503e

[StuartLittlefair]

Yes, yes and a thousand times yes.

The devil will be in the implementation details but this is a thoroughly good idea.

[adrn]

Does anyone else have thoughts before we ask the @astropy/coordinators to review this? cc @eteq @Cadair @ayshih @taldcroft!

[eteq]

I have been traveling the last several weeks and haven't had a chance to review this in detail, but I will try to do so this week. I am fairly sure I will have Opinions ™️ (but hopefully positive ones?)

[adrn]

@eteq OK cool. It'd be good to get your feedback ASAP, because @jeffjennings is starting to work through some of the changes we discuss here (in draft stages, but still, it is somewhat in progress).

[Cadair]

I have two main thoughts about some effects from this change, I think they can both be managed, but probably worth having a think about.

1. The asdf serialisation is going to substantially different from now, managing that transition could be complex. - @braingram
2. Many SunPy frames have an observer attribute which is a realised frame holding the position of the observer. This could obviously become a SkyCoord, but I worry that adds some more complexity, and will for a start make the repr even more obnoxious than it is currently.

[braingram]

1. The asdf serialisation is going to substantially different from now, managing that transition could be complex. - @braingram

Thanks for pinging me about this change. If I'm following the end result will be that coordinate frames will no longer have data.

Reading old files with data when coordinate frames no longer support data

At that point when a user opens an ASDF file with a serialized coordinate frame (which might contain data) the converter that handles the deserialization could see that the file contains data and generate a SkyCoord instead (possibly with a warning that the file should be updated).

Writing files when coordinate frames no longer support data

This should be fine. Technically we could keep the same schema and just never write data (it's not required) but it might be a good opportunity to include some other schema cleanup (at the expense of some schema churn since many schemas reference the base coordinate frame schema).

Implementation details

The details about how this is implemented may impact ASDF serialization. Specifically the first item under implementation:

1. Splitting the frame classes into two hierarchies: ones with and without data.

The converter(s) would need to be updated to support these new classes. This isn't a problem but is work that wouldn't be needed if instead the existing classes were updated (deprecating and then removing the data feature).

3. Deprecating the legacy with-data frame classes.

At this point asdf-astropy could be updated to not use the legacy classes.