Make computing angular separations less surprising by eerovaher · Pull Request #16246 · astropy/astropy

eerovaher · 2024-03-27T15:40:51Z

Description

When computing the angular separation between coordinates in different frames they first need to be transformed into a common frame. However, the resulting angular separation value can depend on the direction of the transformation, unless it is a simple rotation. Users have found this to be surprising, so by default a warning should be emitted if a non-rotation transformation is needed. It should also be possible to always suppress the warning, or to forbid non-rotation transformations.

Computing the position angle can always be done silently because its value is expected to depend on the order of the coordinates anyways.

Closes #8505, closes #12189, closes #11388, closes #14812

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-03-27T15:41:19Z

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.

eerovaher · 2024-03-27T16:11:18Z

I'll write the What's New entry after the code and documentation changes are reviewed.

The RTD build is failing because Sphinx can't figure out how to link to the new type alias I've introduced. This can be addressed by adding a new entry to docs/nitpick-exceptions.

mhvk

Yes, makes sense, since in most cases it may really not be what the user expects (the common case will be that the coordinates are in the same frame, after all).

Comments about the details.

docs/changes/coordinates/16246.feature.rst

astropy/coordinates/baseframe.py

astropy/coordinates/tests/test_separation.py

mhvk

Looks good, only a single suggestion left - but feel free to ignore it!

mhvk · 2024-03-27T19:16:38Z

astropy/coordinates/errors.py

+    def __init__(
+        self, frame_to: BaseCoordinateFrame, frame_from: BaseCoordinateFrame
+    ) -> None:
+        self.frame_to = frame_to


I'd tend to swap these two, but no big deal to keep it as is (I can see the nicety of self, other_frame in the actual initialization...)

I prefer to keep things convenient for the caller, so I'm leaving this as is.

mhvk · 2024-03-27T19:20:17Z

p.s. Personally not sure this needs a what's-new, since in the end this is a relatively small change, but leave that decision up to you.

neutrinoceros

This definitely feels like the right approach, and thank you for testing it extensively. I have 2 suggestions which I can summarize as:

the new public facing argument should be keyword-only
we could use an enum internally to represent valid values for it. Originally I thought of this as a way to enforce exhaustiveness at type-check time, but as I progressed through my review I more or less convinced myself it wasn't worth the effort since it could make it much more awkward to display a nice error message for invalid values, as you did. Anyway, feel free to disregard these comments !

neutrinoceros · 2024-03-28T10:54:01Z

astropy/coordinates/baseframe.py

    from astropy.coordinates import Latitude, Longitude, SkyCoord
    from astropy.units import Unit

+    StrictnessControl: TypeAlias = Literal["ignore", "warn", "error"]


while we're at it using the type system to annotate this, we might as well utilize an actual enum and leverage typing.assert_never

additional notes on this:

typing.assert_never is new in Python 3.11 but available in typing_extensions

maybe surprisingly (but in a good way), it does have an effect at runtime

using aStrEnum would also allow users to pass in plain strings, which would preserve the API you're adding

enum.StrEnum is also new in Python 3.11, but is very light weight so it's easy to backport for 3.10

neutrinoceros · 2024-03-28T10:56:04Z

astropy/coordinates/baseframe.py

+        if not (
+            frame_mismatch == "ignore"


If using an enum as I'm suggesting, this part could be made easier to swallow for type checkers as:

if frame_mismatch is StrictnessControl.IGNORE: pass elif ...: ...

neutrinoceros · 2024-03-28T10:56:50Z

astropy/coordinates/baseframe.py

+            elif frame_mismatch == "error":
+                raise NonRotationTransformationError(self, other_frame)
+            else:
+                raise ValueError(


In passing, assert_never(frame_mistmatch) would also allow runtime validation and elegantly replace this. That said, I recognize that the custom error message is much clearer as is, especially since it's intended to end users. Keeping it doesn't prevent using an enum though.

neutrinoceros · 2024-03-28T11:08:45Z

astropy/coordinates/baseframe.py

+    def separation(
+        self,
+        other: BaseCoordinateFrame | SkyCoord,
+        frame_mismatch: StrictnessControl = "warn",


I feel like since Python 2 was dropped there hasn't been a compelling reason to not make every new arguments to public functions keyword-only (which matches how you're illustrating its use in docs anyway)

Suggested change

frame_mismatch: StrictnessControl = "warn",

*,

frame_mismatch: StrictnessControl = "warn",

Making positional-or-keyword arguments keyword-only is a backwards incompatible change, but making keyword-only arguments positional-or-keyword is backwards compatible. So I agree that it is better to err on the side of caution and make this keyword-only in the public API.

neutrinoceros · 2024-03-28T11:10:06Z

astropy/coordinates/baseframe.py

            The coordinate to get the separation to.
+        frame_mismatch : {"warn", "ignore", "error"}
+            If the ``other`` coordinates are in a different frame then they
+            will have to be transformed and depending on the details


Could you clarify what details ? ("details of the transformation" would be enough, provided that's what you mean here)

neutrinoceros · 2024-03-28T11:17:38Z

docs/changes/coordinates/16246.feature.rst

@@ -0,0 +1,9 @@
+The ``SkyCoord`` and ``BaseCoordinateFrame`` ``separation()`` methods now
+accept an optional ``frame_mismatch`` argument.


Together with my previous suggestion to make the argument keyword-only from the get go.

Suggested change

accept an optional ``frame_mismatch`` argument.

accept an optional ``frame_mismatch`` keyword argument.

neutrinoceros · 2024-03-28T11:20:45Z

docs/changes/coordinates/16246.feature.rst

@@ -0,0 +1,9 @@
+The ``SkyCoord`` and ``BaseCoordinateFrame`` ``separation()`` methods now


It would maybe feel more natural to rephrase this changelog entry as "A warning is now emitted by default in case ... . It is also possible to suppress this warning or to promote it to an error via the new keyword argument ...", instead of introducing the new argument first.

neutrinoceros · 2024-03-28T11:21:58Z

astropy/coordinates/tests/test_separation.py

+
+@pytest.mark.parametrize("coord_class", [SkyCoord, ICRS])
+@pytest.mark.parametrize(
+    "frame_mismatch_kwarg,expectation",


extremely minor but while we're at it

Suggested change

"frame_mismatch_kwarg,expectation",

"frame_mismatch_kwarg, expectation",

Ruff has a few rules for enforcing a consistent style in fixtures and parametrizations, including PT006. I brought these rules up in the developer telecon last September, but the overall opinion was that enforcing them is not worth the effort, so currently contributors are free to use whatever style they like, as long as pytest can parse it.

I use comma separated values (not comma-and-space separated values) because that is what the examples in pytest documentation use.

fair enough !

mhvk · 2024-03-28T14:37:44Z

FWIW, I agree with the suggestion of a keyword-only argument. I like the principle of a StrEnum, but feel that if we go that route, we should do that in a separate PR for more functions (and perhaps only when our minimum python version is 3.11, since really there is no hurry).

eerovaher · 2024-03-28T15:05:58Z

There's a few other places in astropy that allow users to either suppress warnings or to raise errors instead. For example, here's a configuration item in utils that uses the exact same values as frame_mismatch:

astropy/astropy/utils/iers/iers.py

Lines 190 to 191 in 014036d

    
           iers_degraded_accuracy = _config.ConfigItem( 
        
               ["error", "warn", "ignore"],

I agree that using Enum or StrEnum would be good idea, but it should be done consistently throughout astropy so it's beyond the scope for this pull request.

p.s. Personally not sure this needs a what's-new, since in the end this is a relatively small change

This pull request closes 4 issues, and there are several closed duplicate issues. Clearly this is something that is important to the users and deserves to be highlighted.

I'll have to push another commit with the What's New entry, but everything else can be reviewed already.

neutrinoceros · 2024-03-28T16:53:15Z

I like the principle of a StrEnum, but feel that if we go that route, we should do that in a separate PR for more functions (and perhaps only when our minimum python version is 3.11, since really there is no hurry).

There's a few other places in astropy that allow users to either suppress warnings or to raise errors instead.

Dully noted, thank you both. This pattern is growing more and more onto my lately so I'm sure I'll remember to try it out at scale if an opportunity arises.

Anyway, back to this PR: thanks @eerovaher for addressing my comments, I'm happy to sign this off now !

When computing the angular separation between coordinates in different frames they must first be transformed into a common frame. However, the resulting angular separation value can depend on the direction of the transformation, unless it is a pure rotation. Users have found this to be surprising, so by default a warning is now emitted if a non-rotation transformation is needed. It is possible to always suppress the warning, or to forbid non-rotation transformations. Computing the position angle always performs the transformation silently because its value is expected to depend on the order of the coordinates anyways.

eerovaher · 2024-03-28T20:38:41Z

I have made two changes: the frame_mismatch argument has been renamed to the more appropriate origin_mismatch and the documentation now uses "pure rotation" instead of "simple rotation". I have also added a second commit with the What's New entry.

mhvk

This looks all good. The biggest problem is of course naming: you are right that frame_mismatch was not that good -- since some frame mismatches are fine while others are not -- but origin_mismatch to me at first glance suggests that it matters relative to what origin the separation is measured from -- while from the (very nice!) what's new it becomes clear you mean the origin of the coordinate system.

Other options might be "frame_dependent", "ambiguous", "non_associative"...

I'll approve now but won't merge yet in case a better name comes up...

neutrinoceros

I second Marten: this what's new entry is indeed excellent. Just a very minor comment but otherwise I think it's perfect.
Regarding the name of the keyword argument, I didn't think of this before you renamed it but agree that the new name is more accurate. Marten has a point that it may be slightly confusing, though I don't have a better suggestion to make at the moment.

neutrinoceros · 2024-03-29T09:38:17Z

docs/whatsnew/6.1.rst

+Telescope are) the Sun and the Moon will be (almost) in the same direction.
+The :meth:`~astropy.coordinates.BaseCoordinateFrame.separation` method
+automatically converts a coordinate given to it to the frame of the coordinate
+it belongs to, but the separation can be different if the coordinates are


Suggested change

it belongs to, but the separation can be different if the coordinates are

it belongs to, so the separation can be different if the coordinates are

eerovaher · 2024-03-29T16:39:48Z

I merged the pull request that made Cosmology a dataclass and that caused a merge conflict in the What's New file. I solved the conflict by changing the order of the sections in that file and I made some small changes in the process.

mhvk

Thanks!

Fix #16246 change log entry

…333-on-v6.1.x Backport PR #16333 on branch v6.1.x (Fix #16246 change log entry)

eerovaher added coordinates Enhancement whatsnew-needed labels Mar 27, 2024

eerovaher added this to the v6.1.0 milestone Mar 27, 2024

eerovaher requested a review from mhvk March 27, 2024 15:40

github-actions bot added the Docs label Mar 27, 2024

eerovaher force-pushed the separation-frame-mismatch branch from 5d22b5d to df7e169 Compare March 27, 2024 15:42

mhvk reviewed Mar 27, 2024

View reviewed changes

eerovaher force-pushed the separation-frame-mismatch branch from df7e169 to 4015e1f Compare March 27, 2024 17:56

mhvk approved these changes Mar 27, 2024

View reviewed changes

neutrinoceros reviewed Mar 28, 2024

View reviewed changes

eerovaher force-pushed the separation-frame-mismatch branch from 4015e1f to 9ea56b8 Compare March 28, 2024 14:26

neutrinoceros approved these changes Mar 28, 2024

View reviewed changes

eerovaher force-pushed the separation-frame-mismatch branch from 9ea56b8 to e7b9590 Compare March 28, 2024 20:36

eerovaher requested a review from mhvk March 28, 2024 20:42

mhvk approved these changes Mar 28, 2024

View reviewed changes

neutrinoceros approved these changes Mar 29, 2024

View reviewed changes

Add What's New entry for separation origin_mismatch

61a7686

eerovaher force-pushed the separation-frame-mismatch branch from e7b9590 to 61a7686 Compare March 29, 2024 16:29

mhvk approved these changes Mar 29, 2024

View reviewed changes

mhvk merged commit a7c5891 into astropy:main Mar 29, 2024

eerovaher deleted the separation-frame-mismatch branch March 29, 2024 17:11

eerovaher added a commit to eerovaher/astropy that referenced this pull request Apr 23, 2024

Fix astropy#16246 change log entry

4effe74

eerovaher added a commit to eerovaher/astropy that referenced this pull request Apr 23, 2024

Fix astropy#16246 change log entry [ci skip]

049a611

pllim mentioned this pull request Apr 23, 2024

Fix #16246 change log entry #16333

Merged

1 task

pllim added a commit that referenced this pull request Apr 23, 2024

Merge pull request #16333 from eerovaher/fix-coords-changelog

0d4fc36

Fix #16246 change log entry

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Apr 23, 2024

Backport PR astropy#16333: Fix astropy#16246 change log entry

bb2fe1e

meeseeksmachine mentioned this pull request Apr 23, 2024

Backport PR #16333 on branch v6.1.x (Fix #16246 change log entry) #16334

Merged

pllim added a commit that referenced this pull request Apr 23, 2024

Merge pull request #16334 from meeseeksmachine/auto-backport-of-pr-16…

3644edb

…333-on-v6.1.x Backport PR #16333 on branch v6.1.x (Fix #16246 change log entry)

StuartLittlefair mentioned this pull request May 10, 2024

WCS world_to_pixel dependent on SkyCoord distance #16430

Closed

neutrinoceros mentioned this pull request Jun 24, 2024

RFC: use enum.StrEnum where it makes sense, when available #16603

Open

d-giles referenced this pull request in d-giles/astropy-abdufork Jul 26, 2024

Fix #16246 change log entry [ci skip]

a4246ab

	frame_mismatch: StrictnessControl = "warn",
	*,
	frame_mismatch: StrictnessControl = "warn",

		@@ -0,0 +1,9 @@
		The ``SkyCoord`` and ``BaseCoordinateFrame`` ``separation()`` methods now
		accept an optional ``frame_mismatch`` argument.

	accept an optional ``frame_mismatch`` argument.
	accept an optional ``frame_mismatch`` keyword argument.

	"frame_mismatch_kwarg,expectation",
	"frame_mismatch_kwarg, expectation",

	it belongs to, but the separation can be different if the coordinates are
	it belongs to, so the separation can be different if the coordinates are

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

github-actions bot commented Mar 27, 2024

Uh oh!

eerovaher commented Mar 27, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Mar 27, 2024

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Mar 28, 2024

Uh oh!

eerovaher commented Mar 28, 2024

Uh oh!

neutrinoceros commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerovaher commented Mar 28, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Mar 29, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

eerovaher commented Mar 27, 2024 •

edited

Loading

neutrinoceros commented Mar 28, 2024 •

edited

Loading