Repository navigation
Allow BaseCoordinateFrames to be stored in tables (by giving them .info) - #16831
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
I added some in-line comments.
| defaultunit: str | Unit = "recommended" | ||
|
|
||
|
|
||
| class CoordinateFrameInfo(MixinInfo): |
There was a problem hiding this comment.
This is essentially a copy of the old SkyCoordInfo, but with sc replaced by coord and with a single line removed (which gets added back in the new SkyCoordInfo, and with the new _insert method.
|
|
||
| return out | ||
|
|
||
| def _insert(self, obj, values, axis=0): |
There was a problem hiding this comment.
This is essentially the old SkyCoord.insert method - this avoids defining effectively the same method twice. I considered defining a mixin class, but since this method uses _new_like, it seemed reasonable to define it on the info class.
| if "_sky_coord_frame" in self.__dict__: | ||
| if self._is_name(attr): | ||
| raise AttributeError(f"'{attr}' is immutable") | ||
| if attr != "info" and not attr.startswith("_"): |
There was a problem hiding this comment.
The old __setattr__ caused me a lot of trouble, with its setting by default on the frame rather than on the coordinate instance itself. This ensures that info does not get propagated down, and also does a bit more sanity checks before proceeding.
| return | ||
|
|
||
| frame_cls = frame_transform_graph.lookup_name(attr) | ||
| if frame_cls is not None and self.frame.is_transformable_to(frame_cls): |
There was a problem hiding this comment.
This was mirroring what __getattr__ does, but surely for __setattr__ we should just forbid any name that is in the frame transform graph!? (I can change it back, of course).
| cls_full_name = "astropy.coordinates.SkyOffsetFrame" | ||
| elif ( | ||
| cls_full_name not in __construct_mixin_classes | ||
| and not cls_full_name.startswith("astropy.coordinates.builtin_frames") |
There was a problem hiding this comment.
I did not really want to bother to add all the built-in frames explicitly to the white list. We may want to re-think the list more generally. E.g., anything starting with astropy.coordinates probably best is gotten from that directly, since that __init__ is much less likely to contain any other class that might be used to wreck mayhem.
mhvk
changed the title
mhvk
changed the title
|
Very nice! |
|
So, how can the serialization support be extended so that it will also work for SunPy stuff? We have both:
|
|
@ayshih - maybe a better solution than what I have is to check for |
655eb3e to
8bbadfd
Compare
|
@taldcroft - coordinate frames now have a @StuartLittlefair, @adrn - this is the PR that actually uses the length of data-less frames (triggered by @lpsinger's need for storing such frames). |
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
This looks great from the table perspective! To be clear I did not dig into the changes to __setattr__ and __getattr__ here, so someone else needs to check that.
I do wonder if this deserves a What's New entry. It's a nice addition that might otherwise fly under the radar.
The code is almost the same as that for SkyCoord, so base SkyCoordInfo on the new CoordinateFrameInfo. Also, use that class to define an _insert method, which can be used for both BaseCoordinateFrame and SkyCoord.
Includes test for SkyOffsetFrame
4881322 to
382acb2
Compare
|
I added the what's-new, so will merge this now. Thanks! |
This pull request is to address the fact that coordinate frames cannot be written to tables (with or without data). It does this in two commits:
.infoattribute. Here, since the code is almost the same as that for SkyCoord, I baseSkyCoordInfoon the newCoordinateFrameInfo. Also, to avoid duplication of.insertmethods, I define an_insertmethod on the info class, which can be used for bothBaseCoordinateFrameandSkyCoord.BaseCoordinateFramein tables, including tests forSkyOffsetFramewith and without associated data.To get this to work,
Tablecan no longer rely on the input having alen- since data-less coordinate frames raise on that. So, instead it preferscol.shape[0]if that is available. Obviously, instead one couldlen(see #16833), but that seemed a rather big API change (even though it is perhaps reasonable)(EDITED). Furthermore, sinceSkyOffsetFrameclasses are defined on the fly, I treat these as special cases in the white list of allowed classes for serialization.Fixes #16823