Implement `BaseCoordinateFrame.frame` property by eerovaher · Pull Request #16356 · astropy/astropy

eerovaher · 2024-04-29T15:22:28Z

Description

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. Making use of this opportunity in astropy addresses Ruff rule PLR0915 (too-many-statements) in astropy.coordinates.sky_coordinate_parsers._get_frame_without_data() and a UP038 (non-pep604-isinstance) violation in astropy.vizualisation.wcsaxes.core._transform_plot_args().

~~BaseCoordinateFrame.frame could also be useful for implementing a SupportsFrame 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 the visualization maintainers could review the one change I made there.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-04-29T15:22:56Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

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.

mhvk

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.

mhvk · 2024-04-29T21:18:45Z


+    @property
+    def frame(self) -> Self:
+        """A reference to this frame."""


I'd put a comment here that this is for compatibility with SkyCoord.

neutrinoceros · 2024-04-30T08:17:36Z


 class FrameDescription(NamedTuple):
-    frame: BaseCoordinateFrame
+    frame: BaseCoordinateFrame | SkyCoord


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.

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.

that's what I'm proposing too. A protocol would document exactly which parts of these classes are needed here.

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).

neutrinoceros · 2024-04-30T08:26:13Z

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 BaseCoordinateFrame more like SkyCoord as it visibly helps with reducing coupling in a couple places that can now stay blissfully unaware what exact type they are being passed. As far as type annotations go I think defining an actual Protocol would be useful documentation for what exactly are the important common interfaces between these two classes, but other than that LGTM.

Cadair · 2024-04-30T08:38:26Z

UP038 are particularly useful rules to enforce

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 | operator) and according to ruff it's slower! "Note that this results in slower code.". So UP038 is one of the few rules that live in my ignore config permanently.

Cadair · 2024-04-30T08:42:47Z

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?

neutrinoceros · 2024-04-30T08:45:29Z

I personally have a the exact opposite opinion to this

@Cadair did you miss the negation in my sentence ? I think we actually agree !

Cadair · 2024-04-30T09:02:59Z

Haha yes. Don't read GH until after coffee lol

eerovaher · 2024-04-30T22:06:23Z

I've expanded this pull request by implementing the SupportsFrame protocol.

@mhvk commented

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)

The code can indeed be simplified with or without implementing BaseCoordinateFrame.frame, but at least mypy seems to have some trouble figuring out what the return type of getattr(other, "frame", other) is, so for type checking the ability to simply access other.frame is much better.

...am a little unsure about the implicit API change, that anything that has a .frame attribute will now work a bit like a SkyCoord

We can get better duck typing both by implementing BaseCoordinateFrame.frame or by using getattr(other, "frame", other), but it will be much simpler to specify what is required for duck-typing by implementing a protocol, and then there is little reason why BaseCoordinateFrame shouldn't implement that protocol.

@neutrinoceros wrote

As far as type annotations go I think defining an actual Protocol would be useful documentation

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 astropy is very small.

@Cadair commented

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?

In astropy the classes that implement the SupportsFrame protocol are SkyCoord and BaseCoordinateFrame, but perhaps some users would like to have some custom class that does not inherit from either, and now with the protocol in place it starts being possible.

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`.

eerovaher · 2024-05-02T18:12:55Z

You can have a look at how SupportsFrame is documented.

astrofrog

The visualization change looks good to me

eerovaher · 2024-09-05T16:34:05Z

Before any specific code changes are reviewed, we should first decide if we want to define coordinate-like objects using a Protocol or not. The way I see it, that question is equivalent to the question whether we should support duck-typing coordinate-like objects or not.

github-actions · 2024-09-30T05:44:27Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

eerovaher · 2024-10-02T18:42:40Z

From the point of view of code simplicity it would be best if there was a single type that would encompass all coordinate-like objects. The choice we need to make is whether that single type should be a Protocol or a class. But if we choose a class then it should be a single class, i.e. either SkyCoord or BaseCoordinateFrame but not both. It should be the users responsibility to either wrap all BaseCoordinateFrame instances in a SkyCoord or to unwrap the BaseCoordinateFrame using SkyCoord.frame.

neutrinoceros · 2024-10-25T13:18:05Z

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 astropy is very small.

Actually, and as you've since pointed out yourself (I forget where) Mixin Columns are an example of protocol that already exists in documentation, if not formalized in code (yet). Could we do something similar here ?

github-actions · 2024-10-31T05:40:13Z

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

eerovaher added coordinates Enhancement visualization.wcsaxes labels Apr 29, 2024

eerovaher added this to the v7.0.0 milestone Apr 29, 2024

eerovaher requested review from Cadair and mhvk April 29, 2024 15:22

eerovaher requested review from astrofrog and larrybradley as code owners April 29, 2024 15:22

github-actions Bot added the Docs label Apr 29, 2024

eerovaher force-pushed the basecoordinateframe_frame branch from c88e434 to d93aa0e Compare April 29, 2024 15:27

eerovaher removed the Docs label Apr 29, 2024

mhvk reviewed Apr 29, 2024

View reviewed changes

neutrinoceros reviewed Apr 30, 2024

View reviewed changes

Comment thread astropy/coordinates/tests/test_exceptions.py Resolved

neutrinoceros reviewed Apr 30, 2024

View reviewed changes

Comment thread astropy/coordinates/tests/test_exceptions.py Resolved

eerovaher force-pushed the basecoordinateframe_frame branch from d93aa0e to c968c1a Compare April 30, 2024 20:59

neutrinoceros approved these changes May 1, 2024

View reviewed changes

eerovaher added 2 commits May 2, 2024 19:54

Implement SupportsFrame protocol

e69cbf0

The new protocol specifies objects that contain coordinate data as a `BaseCoordinateFrame` instance in their `frame` property, e.g. `SkyCoord` and `BaseCoordinateFrame` itself.

eerovaher force-pushed the basecoordinateframe_frame branch from c968c1a to 240f1d7 Compare May 2, 2024 17:55

astrofrog approved these changes Sep 5, 2024

View reviewed changes

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Sep 30, 2024

saimn modified the milestones: v7.0.0, v7.1.0 Oct 25, 2024

github-actions Bot added the closed-by-bot Closed by stale bot label Oct 31, 2024

github-actions Bot closed this Oct 31, 2024

eerovaher mentioned this pull request Nov 4, 2024

APE26: Removing data storage (representations) from coordinate frames astropy/astropy-APEs#112

Merged

Conversation

eerovaher commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Apr 29, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Apr 30, 2024

Uh oh!

Cadair commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cadair commented Apr 30, 2024

Uh oh!

neutrinoceros commented Apr 30, 2024

Uh oh!

Cadair commented Apr 30, 2024

Uh oh!

eerovaher commented Apr 30, 2024

Uh oh!

eerovaher commented May 2, 2024

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Sep 5, 2024

Uh oh!

github-actions Bot commented Sep 30, 2024

Uh oh!

eerovaher commented Oct 2, 2024

Uh oh!

neutrinoceros commented Oct 25, 2024

Uh oh!

github-actions Bot commented Oct 31, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eerovaher commented Apr 29, 2024 •

edited

Loading

Cadair commented Apr 30, 2024 •

edited

Loading