Ensure BaseCoordinateFrame separation methods work with SkyCoord input

[eerovaher]

Description

The first commit here adds a test which demonstrates that it is possible to get wrong results from BaseCoordinateFrame separation() and separation_3d() methods if the input to those methods is a SkyCoord instance. These methods do not claim to support SkyCoord input, but if they don't work with SkyCoord they should fail in an obvious manner. The current behavior is therefore a bug and the second commit here fixes it.

The third commit removes the SkyCoord separation() and separation_3d() methods. This can be removed without loss of functionality after fixing the bug because a SkyCoord instance then exposes the separation methods of its underlying frame.
EDIT: there will be a separate pull request for the third commit.

• 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]

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?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• 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.

[neutrinoceros]

while we're at it maybe we could also chain these exceptions either as

try:
    ...
except TypeError as exc:
    raise TypeError(...) from exc

or

try:
    ...
except TypeError:
    raise TypeError(...) from None

? (I think the second one is more appropriate here)

[eerovaher]

It doesn't look like this code path can ever be triggered. Trying to compute the separation of a frame without data raises a ValueError, not a TypeError:

>>> from astropy.coordinates import FK5, SkyCoord
>>> SkyCoord(1, 1, unit="deg").separation(FK5())
Traceback (most recent call last):
  ...
ValueError: Cannot transform a frame with no data

To me it seems that this error message is clear enough, so this try-except block can be removed.

[mhvk]

Really good to simplify this! I have one name suggestion, but also a general question: do we even need these methods at all? If we don't define them, one falls through to the equivalent methods on the frame - what goes wrong if one uses those? It looks like they do the same thing...

[eerovaher]

I had a brief look at code performance. Setup:

Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.15.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from astropy.coordinates import SkyCoord, ICRS

In [2]: from astropy import units as u

In [3]: sc1 = SkyCoord(1 * u.deg, 2 * u.deg, 1 * u.kpc, frame="galactic")

In [4]: sc2 = SkyCoord(2 * u.deg, 3 * u.deg, 4 * u.kpc)

In [5]: icrs = sc2.frame

With 6b36e53 (the commit from which this pull request branched):

In [6]: %timeit sc1.separation(sc2)
1.05 ms ± 17.4 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [7]: %timeit sc1.separation(icrs)
897 µs ± 27.4 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [8]: %timeit sc1.separation_3d(sc2)
602 µs ± 10.4 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [9]: %timeit sc1.separation_3d(icrs)
463 µs ± 15.9 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

With c00bca0:

In [6]: %timeit sc1.separation(sc2)
718 µs ± 9.77 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [7]: %timeit sc1.separation(icrs)
712 µs ± 3.12 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [8]: %timeit sc1.separation_3d(sc2)
472 µs ± 6.66 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [9]: %timeit sc1.separation_3d(icrs)
470 µs ± 1.59 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

[mhvk]

p.s. Somewhat surprising, the increase in performance. But nice!

[neutrinoceros]

is there something wrong with codecov reports lately or should a test or two be added to cover these (surprisingly) uncovered paths ?

[pllim]

There was an error 404 in the upload step in the coverage job here but somehow codecov does not fail the job. When you see weird numbers, go into the log and check the upload step. This happens on and off because pull_request does not get the token to guarantee success. If codecov thinks we're spamming them, this happens.

Unless coverage check is critical, I usually ignore. Otherwise, maintainer can opt to rerun just that job.

The non-zero number on failure is because CircleCI also uploads coverage but only for visualization tests.

Hope this clarifies the matter!

[eerovaher]

The changes this pull request now makes to BaseCoordinateFrame allow its separation methods to handle SkyCoord instances as arguments, whereas previously that was not always the case. That might require a change log entry. I'll leave it to reviewers to remove the no-changelog-entry-needed label if they agree.

[mhvk]

There's part of me that worries about omitting all the SkyCoord non-frame attributes, but since that was the case already, clearly it is not something that is obviously a problem (and it is clear that just merging attributes is a problem).

I do think we should have a changelog entry, since really the old behaviour for Baseframe was a bug for SkyCoord input.

[eerovaher]

Updates to the BaseCoordinateFrame separation methods fix a bug and should therefore be backported, but it is not clear to me if the third commit here that removes the SkyCoord separation methods should also be backported. I have included it here for now so that all the changes could be reviewed together, but I am expecting that it requires a separate pull request and should be removed from here before merging.

[mhvk]

To me, this looks all good and since there is no actual change in SkyCoord.separation* - from its perspective, this is just a refactoring - it seems fine to me to merge as is, including the backport. But I won't do that yet just in case others disagree; I'm fine with not backporting or splitting as well.

[eerovaher]

@pllim, could you advise if the refactoring commit should be included here, in which case it will be backported with the bugfix, or if a separate pull request should be opened for the refactoring?

[pllim]

We generally do not backport refactoring. But if it is just a few lines and totally backward compatible, maybe ok.

tl;dr -- new PR would be best

[mhvk]

OK, thanks for splitting the bug off! Looks all OK.

[mhvk]

Darn, I merged before checking for the backport label. Was there a simple phrase to get the bot to still do the backport for us?

[eerovaher]

@meeseeksdev backport to v6.0.x

[eerovaher]

I should have remembered to add the backport label myself, but luckily there is a simple phrase the bot responds to.