Deprecate coordinates.get_moon()

[eerovaher]

Description

get_moon() is simply a shortcut to calling get_body("moon") with a docstring that tends to get out of sync from the docstring of get_body() (as evidenced by #12683), and the presence of which complicates unit tests in astropy.coordinates.tests.test_solar_system because Moon is treated differently from all the other Solar System bodies. Deprecating and eventually removing it simplifies the code without sacrificing functionality or readability. Note that this pull request removes a net 22 23 lines of code despite adding a deprecation test and a change log entry and get_moon() hasn't even been removed yet.

It seems to me the reason a separate function was created for the Moon in the first place was because it was introduced in 4a59bb8 together with a function called get_planet(), and calling get_planet("moon") the equivalent of get_planet("moon") would have been confusing. But get_planet() was renamed to get_body() already in cd408c2 (committed on the same day). I don't understand why get_moon() was not removed then.

[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 "When to rebase and squash commits".
• 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.

[bmorris3]

Hi @eerovaher!

But get_planet() was renamed to get_body() already in cd408c2 (committed on the same day). I don't understand why get_moon() was not removed then.

I implemented a convenience function called get_moon, even though it was only a tiny helper wrapper, in part to keep special parity with the get_sun method.

A bit of history: my original get_planet implementation in astropy used indices to identify each solar system object (discussion), and specifying the Moon as an integer (especially 3) seemed confusing.

Unlike get_moon, get_sun has a special right to exist independent of get_body/get_moon because it uses the erfa routines to get the solar position rather than jplephem like get_body:

astropy/astropy/coordinates/funcs.py

Lines 150 to 154 in 01d2839

	The algorithm for determining the sun/earth relative position is based
	on the simplified version of VSOP2000 that is part of ERFA. Compared to
	JPL's ephemeris, it should be good to about 4 km (in the Sun-Earth
	vector) from 1900-2100 C.E., 8 km for the 1800-2200 span, and perhaps
	250 km over the 1000-3000.

TL;DR: get_moon doesn't need to exist, and I agree that it's clearer if it does not exist!

[bmorris3]

Looks great!

[bmorris3]

Suggested change

	``get_body("moon")``.
	``get_body("moon", time, [location])``.

[eerovaher]

I've changed the phrasing of the change log entry, although I tried to make the example function calls less shell-like and more Python-like.

[StuartLittlefair]

I'm happy to approve this in principle, since there really is no need for get_moon to exist.

However, it does make me question why we don't deprecate get_sun at the same time? @bmorris3 isn't right about one using erfa and the other using jplephem. get_body uses whatever the solar system ephemeris is set to, and the default is to use erfa's ephemeris.

Therefore, by default both get_body and get_sun use erfa.epv00 to find the relative position of the Earth and the Sun. They differ in how the effects of aberration and the light travel time are handled. I'm not sure there's justification for having two different implementations of the same thing?

[eerovaher]

I've found that removing get_sun() in favor of get_body("sun") was already discussed in #4952. In #4952 (comment) @eteq said that get_sun() should not be removed, but I haven't been able to find what the justification for that statement was. I am somewhat hesitant about deprecating get_sun() before I understand why it wasn't done already at the time of #4952.

[StuartLittlefair]

Interesting, I’d like to hear @eteq ’s view on why get_sun should survive.

Either way, given that there is definitely no reason to keep get_moon and the title of this PR is lunar only, I’ll approve this and open a different issue to propose deprecating get_sun

[mhvk]

Agreed that get_moon can be deprecated. Just a small comment on another line that can be removed.

[mhvk]

Note that the definition of de432s_separation_tolerance_moon can also be removed (and it is the same as for planets, so the tests indeed stay the same)

[eerovaher]

I've removed de432s_separation_tolerance_moon.

[eerovaher]

@mhvk, I've implemented your suggestion. Can this be merged now?

[mhvk]

Oops, completely missed that, thanks for pinging. Yes, let's get this in!