Unconventional definition/documentation of `to_polar`

[jrs65]

Spherical polar coordinates (r, theta, phi) are defined such that z = r cos(theta), i.e. theta is an angle measured from the polar axis to the vector pointing at the given position. The routine to_polar returns the latitude instead (i.e. the angle from equatorial plane). This is unexpected from the name, and also violates the cited ISO-31-11 which defines spherical coordinates in the standard manner.

There's only seven calls in total to to_polar so I don't think it would be that much of an issue to change them all to some new convention. Given that the existing call basically calculates the most useful quantities for astronomical coordinates it probably makes sense to use that, but call it something other than to_polar (e.g. xyz_to_radec). I'd probably rearrange the return value order to (r, ra, dec) (as opposed to the currentr, dec, ra) which turns it back into a right handed coordinate system.

I can probably fix these and send in a PR if this makes sense.

[brandon-rhodes]

Good question! Skyfield uses the convention from Physics, not astronomy; for the difference, see:

https://en.wikipedia.org/wiki/Spherical_coordinate_system

I had hoped to stave off confusion on this point by referencing the relevant ISO standard, which specifies the meaning and exact order of the parameters, right in the docstring; I'll go expand the docstring with more explanation since it didn't wind up being enough. I'll include the wikipedia link, too. Let me know if there are other links or phrases that might help folks keep straight which convention has been chosen — thanks!

[jrs65]

Sure, but the problem is that to_polar doesn't actually use the convention from physics. It gets the polar angle incorrect. If it was using the convention from physics functions.py:L56 should be theta = arccos(z / r).

[brandon-rhodes]

Ah — so you're also concerned about the difference between the astrometric way of measuring "latitude" (up and down from the equator), versus the approach of measuring it from the pole?

[jrs65]

Exactly. Only the latter set would be considered correct spherical coordinates by the ISO standard or the "physics convention". In particular it's super confusing to label what is technically latitude theta.

[brandon-rhodes]

I'll re-open this issue and at the very least document more clearly the difference. I can't change the call signature without breaking folks's working code, I don't think.

[brandon-rhodes]

Oh, I know what I can do. Introduce routines with spherical in the name instead of polar and have them do the right thing; then mark the polar ones as deprecated :)

I'll look at what it would do the code.

[jrs65]

Great. I think that solution makes a lot of sense, I wasn't thinking about the case that folks might make use of that routine externally!

[brandon-rhodes]

Looking at all of the uses of the polar routines in the code, it looks to me like having the spherical routines deal with theta will just generate work everywhere, since what all the outer routines need is a latitude-ish number, not a number measured from the pole. So I'm likely to name the parameters in and out of the new routines lat and lon unless we can come up with something better (I thought for a minute about longish and lattish but that would be difficult to parse.)

[brandon-rhodes]

And if I do that I wonder if I should put distance last, to match everything else? If I'm not doing ISO, why not match everything else in the code?

Any votes one way or the other?

[brandon-rhodes]

(See the commit, above, for my decision and rationale. Thanks!)