-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement BaseCoordinateFrame.frame
property
#16356
base: main
Are you sure you want to change the base?
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.
|
It is not uncommon to have code that extracts (or could extract) the coordinate data from a `SkyCoord` as a `BaseCoordinateFrame` by accessing the `frame` property of the `SkyCoord`. Such code can now handle both `SkyCoord` and `BaseCoordinateFrame` instances in a uniform manner.
c88e434
to
d93aa0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit of two minds on this. On the one hand, I like the simplifications in the code, but on the other hand it would seem like those mostly could be reached just as well by judicious use of getattr(other, "frame", other)
followed by an isinstance
check. I also , I don't really like properties that return self
and am a little unsure about the implicit API change, that anything that has a .frame
attribute will now work a bit like a SkyCoord
- though my general inclination towards ducktyping makes this less of an issue.
Given that I'm on the fence, maybe best to wait and see what others think.
@@ -658,6 +660,11 @@ def getter(self): | |||
setattr(cls, private_attr, value) | |||
setattr(cls, attr_name, property(getter, doc=doc)) | |||
|
|||
@property | |||
def frame(self) -> Self: | |||
"""A reference to this frame.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put a comment here that this is for compatibility with SkyCoord
.
) | ||
|
||
if TYPE_CHECKING: | ||
from astropy.coordinates import BaseCoordinateFrame | ||
|
||
|
||
class FrameDescription(NamedTuple): | ||
frame: BaseCoordinateFrame | ||
frame: BaseCoordinateFrame | SkyCoord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, what matters is what these classes have in common, so I would suggest to formally define this common base as a typing.Protocol
and use it here instead of an Union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like that @eerovaher has kept to the simple & readable for now; that seemed to be the conclusion too from the long discussion we had on typing, that we use it first mostly as documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what I'm proposing too. A protocol would document exactly which parts of these classes are needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it might well be less readable - even a simple SkyCoordLike
is less clear than just listing two options.
Anyway, it is no big deal, I just want to avoid overzealous typing when the benefits are unclear (and it seems obvious that the whole ecosystem is still rapidly evolving).
The description got me on the fence too because I don't think PLR0915 and UP038 are particularly useful rules to enforce and I was worried that was the whole motivation for this PR. However, reading through the diff I appreciate the idea of making |
I personally have a the exact opposite opinion to this, I think it's entirely pointless (a tuple is just as clear as a union operator), it dosen't play nicely with models (as they override the |
I am also on the fence about this, I don't like it from an API clarity point of view, it's really weird. I also don't know how many people in the wild would actually find this useful (I suspect it will be handy in sunpy in the same way it's handy internally for astropy) as we really don't recommend using coordinate frame objects over SkyCoord anywhere? |
@Cadair did you miss the negation in my sentence ? I think we actually agree ! |
Haha yes. Don't read GH until after coffee lol |
d93aa0e
to
c968c1a
Compare
I've expanded this pull request by implementing the
The code can indeed be simplified with or without implementing
We can get better duck typing both by implementing
The reason I didn't implement the protocol when I opened the pull request was that I wasn't sure how it should be documented. We are currently not documenting protocols in the user documentation (we don't really have any to document) and the utility of the protocol in type annotations is limited by the fact that the fraction of annotated code in
In |
The new protocol specifies objects that contain coordinate data as a `BaseCoordinateFrame` instance in their `frame` property, e.g. `SkyCoord` and `BaseCoordinateFrame` itself.
Previously `NonRotationTransformationError` and `NonRotationTransformationWarning` expected its `frame_to` and `frame_from` parameters to be `BaseCoordinateFrame` instances, but now they will work with anything that implements the `SupportsFrame` protocol, most notably `SkyCoord`.
c968c1a
to
240f1d7
Compare
You can have a look at how |
Description
It is not uncommon to have code that extracts (or could extract) the coordinate data from a
SkyCoord
as aBaseCoordinateFrame
by accessing theframe
property of theSkyCoord
. Such code can now handle bothSkyCoord
andBaseCoordinateFrame
instances in a uniform manner. Making use of this opportunity inastropy
addresses Ruff rule PLR0915 (too-many-statements) inastropy.coordinates.sky_coordinate_parsers._get_frame_without_data()
and a UP038 (non-pep604-isinstance) violation inastropy.vizualisation.wcsaxes.core._transform_plot_args()
.BaseCoordinateFrame.frame
could also be useful for implementing aSupportsFrame
protocol, but I'm not planning to do that yet.EDIT: I've implemented the protocol, lets see which way the discussion goes.
Most of the changes in this pull request are in
coordinates
, but it would be good if thevisualization
maintainers could review the one change I made there.