Like this, @bsipocz? Thanks for the help, sorry to be learning by experimentation.

yes, this looks better :)

Planet index seems a little obscure. Although most of us have the planet order memorized, what if someone finds Planet X between two other existing ones? Might be more descriptive to use explicit strings like 'mercury', 'earth', etc.

I completely agree with this - Pluto has taught us the planet indices are not fixed! String name would be better. Or planet/dwarf planet unicode (☿, ♀, ♁, ♂, ♃, ♄, ♅, ♆, ♇) 😉

Hmm, I for once fall on @adrn's side against string parameters, and think it's faster and more convenient to type integer indices, especially for a problem like this where indices are rather obvious (planet 4 is the fourth planet). Would anyone be opposed to really simple parser that takes ints or strs, and maps strings to integers if they're given?

If Pluto doesn't sit well, we can limit the input arguments to integers 1-8 and make a separate function to expose Pluto, since you get it for free with jplephem.

How about a half-way compromise using module constants?

MERCURY = 1
VENUS = 2
...

This way, user can use solar_system.MERCURY as an input to the function.

I would just have

PLANETS = {'mercury': 1,
...
}

and then in the routine proper something like:

index = PLANETS.get(index, index)

My quick proposal was just that if index was in the dictionary, it would return the associated value, if not, it would just be kept as is. So, if you pass in an integer, it stays as is. Of course one could be more elaborate

if isinstance(index, six.string...):
    index = PLANETS[index]

And then catch the KeyError, etc.

Anyway, bottom line is that I don't see much of a problem with supporting both strings and integers (as long as the meaning of the integer is the same as it is in the ephemeris file).

@mhvk, fine by me!

I definitely think we want to disallow 9 though. Also, +1 to supporting strings too (this is not going to dominate performance)

In case it helps: note that there is already a standard set of integer indexes for planets, long maintained as part of the SPICE package from NASA:

https://github.com/mattbornski/spice/blob/master/src/zzidmap.c

The jplephem package makes these available in Python in its jplephem/names.py module, if you want to re-use them.

Also +1 to allow strings. That's much more in line with what other pythonic tools do. But no harm in allowing ints or strings.

Are you sure 3 is the Moon? I haven't had time to look carefully, but I thought it usually encoded the earth-moon barycentre. In any case, all the more reasons to allow input using a string!

I will be the first to admit that this interface is confusing. What's going on here is that it would be senseless to create a SkyCoord for Earth, so I want to prevent users from selecting planet_index=3 for the Earth. Then when I wrote the get_moon function, I didn't want to duplicate the code that accesses the DE430 kernel for both the planets and for the moon, so I wrote the underlying functions in get_planet to give the coordinates of the moon in the special case of planet_index=3. See the one-line get_moon implementation here.

This design choice isn't a weird relic from the weirdness of the DE430 kernel, it's me trying to write code that's not redundant. I'm open to alternatives to make this clear or more intuitive.

If we're using indices that match those used by the JPL kernel, 3 should have the same meaning as those have, otherwise it is just going to be too confusing in my opinion (especially since erfa.plan9 does the same thing!).

p.s. It is sensible to get a position of the Earth relative to the solar system barycentre, since this underlies any precise barycentric corrections.

I understand where you're coming from. I was only designing a function for the purposes of creating SkyCoord objects, since my selfish motivations are to use these functions in astropy/astroplan#35 and astropy/astroplan#111.

match those used by the JPL kernel, 3 should have the same meaning as those have

For the purposes of this simple get_planet as a SkyCoord function, I am in favor of avoiding using the jplephem lexicon for the integers that represent planet barycenters and planets because they are non-intuitive and unnecessary to know for the average user trying to get the position of a planet as a SkyCoord.

I think part of the reason why it took so long for a PR like this to arrive is because the JPL kernel indices are confusing and messy. Hiding that mess was my top priority in this PR.

Well, for me, the reason I did only do a kludge myself is that I don't care where the planets are, but do care about very accurate barycentric corrections...

Anyway, I'm happy with unambiguous entries, but think it is only confusing if you use something that is almost but not exactly the same as the way the ephemeris files (and plan9) are organised.

I just feel like it is a waste not to include the following in this function:

if some_condition:
    warnings.warn('That is no moon!', AstropyUserWarning)

But, Pluto is not a planet anymore...

I thought about this and decided that it's still pretty clear what 9 represents. Does anyone think that pluto should be inaccessible from the get_planet function?

This is a tricky question. Because Pluto is still a planet in New Mexico.