Replies: 4 comments 21 replies
-
Thanks for starting a discussion about this feature! I will be traveling next week and so probably won’t have more to say until late October, but:
I'll share any further thoughts if I have them within the next couple of days; otherwise, look for more from me later in October! |
Beta Was this translation helpful? Give feedback.
-
Sounds reasonable. I would say if latlon's third return is the same as and
height_ASL, it would be best to make sure the documentation and code are
consistent in terminology, whether it's heigh_ASL, altitude, or elevation.
…On Thu, Nov 4, 2021, 6:12 PM bismurphy ***@***.***> wrote:
So maybe the best way is to have:
1.
get_subpoint returns a point which is at sea level, at the same
lat/long as the given position
2.
to_latlon returns the position's coordinates, in lat long and altitude
(unfortunately both "altitude" and "elevation" are used along with azimuth
to describe observer-centered celestial coordinates).
3.
Introduce new function height_ASL which gets the position's height
above sea level - which is one of the outputs of to_latlon, but breaks
it out explicitly for the use case that doesn't care about lat/lon, since
as Patrick said, it's good to make sure users are aware of what functions
are at their disposal.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#644 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC4ANH3Z5C727ZHNIO5PKRLUKMVQNANCNFSM5FPZPUDA>
.
|
Beta Was this translation helpful? Give feedback.
-
I wonder if that’s too heavyweight. Maybe there should be a latlon() method
that only returns lat and lon, and skips the elevation computation? Maybe
an elevation method should exist that skips computing anything else? Users
who called both would duplicate some calculations, but we could always
warn about that in the docs.
This is what I was thinking. Provide GeographicPosition object that calculates all three at once, but also provide methods for only returning the latlon or the elevation. I think the first concern should be accessibility, ease of finding the methods. After that, anyone trying to optimize performance should be expected to have read the docs in enough detail to know that they should use the Geographic Position for optimal performance.
And that left “elevation” as the word to height-above-sea-level — with the
added bonus of avoiding the fact that “altitude” is ambiguous because of
the distinction between AGL and MSL, a distinction that (happily) doesn’t
seem to exist for “elevation”.
Wow. These terms really are fraught with a variety of usage and ambiguity. I've actually never heard of altitude being used as degrees above the horizon. All my astronomy experience has been with elevation used for that. What if you select terms that would be unambiguous and whatever they are just make sure to spell them out clearly in the documentation? Alternatively, you could just use the least ambiguous term possible, like height above sea level.
my_satellite.subpoint(…) # subpoint of the satellite
This would definitely be far more ideal for the subpoint method. It would be much clearer, that the subpoint is a property of the satellite. Can you not pass the wgs84 as a parameter of the subpoint method?
…On Wed, Nov 10, 2021 at 9:22 AM, Brandon Rhodes < ***@***.*** > wrote:
>
>
> This is definitely personal preference, and I'm sure any naming would be
> fine, but for me putting "of" at the end of a function name seems odd.
>
>
It’s true that not all libraries use _of. With it, I’m attempting to
signal whether a method returns a value intrinsic to an object itself, or
whether it returns a fact about some other object. In retrospect, I now
think the name wgs84.subpoint(…) is misleading because, when encountered in
code, it makes it sound (until you stop and study the argument) like WGS84
itself has a subpoint, somewhere on the earth, whose position is being
computed.
Grammar would make me expect a subpoint() method, without _of() in the
name, to compute the subpoint of the thing itself, like:
my_satellite.subpoint(…) # subpoint of the satellite
jupiter.subpoint(…)
# subpoint of Jupiter
saturn.subpoint(…) # subpoint of Saturn
And so forth. “I want this thing’s subpoint.” But in the case of a geoid,
we aren’t asking for the geoid’s subpoint, but rather are asking the geoid
to compute the subpoint “of” some other object.
Skyfield already uses _of() as a function and method suffix in a few other
places, so it will have some symmetry with an existing practice; it
offers, per the above, greater clarity; and of course it gives us a new
name for this new functionality that lets us quietly leave the old method
in place (though deprecated) for folks who were using it.
>
>
> I think it might flow better to have … get_subpoint
>
>
Alas, there are two problems with get_.
One is symmetry, or consistency: if this method is prefixed with get_ ,
then arguably most Skyfield methods will need to be prefixed with get_.
All of the dozens of Time methods, for example, would need the prefix
added.
There are indeed languages where most methods start with get_ (Java, for
example), and whose code thereby reads a bit more like an English
sentence. But that’s not the custom in Python, even though it sometimes
makes it feel grammatically like a word is missing. Here, I’ll choose to
be constrained by the pattern already established by other Skyfield
classes, and leave get_ off.
>
>
> I think it might flow better to have to_latlon
>
>
The prefix to_ has the same problem with get_ : to be consistent, I would
have to have it everywhere. (All the Time methods would need it, for
example, just as with get_.)
But as for the choice of whether to mention “elevation”, I think you’ve
brought up a very important point, which identifies an inconsistency in my
approach!
By using latlon in the name, I was following the convention of Skyfield
position methods like radec() and altaz() : the method names the
coordinates that it returns. I have carefully crafted Skyfield so that if
you want RA and declination, you can just type.radec() instead of having to
remember that they are called “equatorial coordinates” and then having to
remember the names of the variables that should receive them ( Ra and dec ).
So I was thinking of latlon() as symmetric with something like radec , that
needs to name the values it returns.
But it’s not symmetric!
The radec() method and altaz() method return coordinate tuples that the
user can unpack and use separately:
ra, dec, distance = p.radec()
alt, az, distance = p.altaz()
But that’s not what latlon() does — and I hadn’t even noticed! It doesn’t
return separate coordinates:
lat, lon, elevation = wgs84.latlon_of(p) # raises error!
Instead, it returns a whole new GeographicPosition object — a single object
instead of three. The user then needs to go look at the object’s named
attributes to get the three separate coordinates. So there’s no mystery
about names. Instead of an anonymous tuple of three elements that have to
be “caught” in the right order by an assignment, the caller will
explicitly ask for its.latitude or.longitude or.elevation.
So there’s a strong argument that putting latlon in the method name is
misleading, since it might mislead folks into trying to unpack the return
value into a lat and lon just like they’d do with.radec().
Should it be named for what the kind of object it returns?
# Some variation of:
g = wgs84.geographic_position(p)
g =
wgs84.geographic_position_of(p)
g = wgs84.to_geographic_position(p)
That looks very verbose to my eyes. While something like geoposition would
be briefer, it’s not as easy to read. And it introduces the whole
dichotomy between “what I want (lat and lon)” and “what I need to ask for
(some class that Brandon named GeographicPosition )” that I’ve tried to
avoid so far in Skyfield’s design.
I wonder if a GeographicPosition should have been a LatitudeLongitudeElevation
instead? But, when I was writing it, it felt like “geographic” was so
close to meaning “longitude and latitude” that I hoped folks wouldn’t
stumble much in using it.
I’m open to ideas here. What method name might most clearly say “this
doesn’t return two or three separate coordinates, it returns a GeographicPosition
object with a longitude and latitude (and, if you care, an elevation)”?
>
>
> It seems like conceptually converting to latitude and longitude (plus
> elevation) is just a change of coordinates while getting the subpoint is a
> new position entirely
>
>
That’s an excellent point — the subpoint is a new position, whereas the
other merely restates the same position in different coordinates.
I’m not sure that the difference between to_ and get_ , in my particular
eyes, quite rises to the distinction between “another version of the same
thing” and “something new” (but maybe I’m too accustomed to Java code
where get_ is merely how you pull an object’s attributes?), but the
distinction in this case is definitely an important one that we’d like the
naming to catch. I’m open to further discussion on how these various names
do and don’t manage to communicate that difference!
>
>
> [ @ patrickwalton ( https://github.com/patrickwalton ) ] I was confused
> about this at first read, but I realize now that the "elevation" given by
> "latlon_and_elevation_of" is the distance of a celestial object above sea
> level. I believe that's an unclear usage for an object that isn't in
> contact with the ground. At least, in the case of a satellite, "altitude"
> should be used…
>
>
Unfortunately Skyfield doesn’t know which targets are above the ground,
and which are on the ground itself. Latitude, longitude, and an offset
from geoid-sea-level are most often used in Skyfield to describe the
position of an observatory or of a private observer — both of which are
usually (but not always!) attached to the ground.
When designing Skyfield, I had to deal with two ambiguous terms:
“altitude” and “elevation”. They can both be used for “height above sea
level” and both for “angle above or below the horizon”. I broke the tie
this way: since I’ve heard of an “altaz” telescope mount before, but never
an “elaz” mount, I decide that angle-above-the-horizon would win the name
“altitude” and be one of the return values of the.altaz() method. And that
left “elevation” as the word to height-above-sea-level — with the added
bonus of avoiding the fact that “altitude” is ambiguous because of the
distinction between AGL and MSL, a distinction that (happily) doesn’t seem
to exist for “elevation”.
Having said that, I freely admit that the choice does indeed introduce a
bit of awkwardness when talking about satellite positions many kilometers
above the Earth, since in the satellites field “altitude” is, exactly as
you say, the common term. But the awkwardness hasn’t yet tempted me to
introduce the term, and with it the above two ambiguities, into Skyfield.
Maybe if I did satellite work I’d have chosen differently.
I’ll note that the question of “altitude” versus “elevation” can be also
discussed separately from this issue about the “subpoint” method — the GeographicPosition
class could conceivably grow an.altitude synonym for its existing.elevation
attribute for satellite folks to use. More extreme would be to attempt to
carve it into two classes, one of which offers an.altitude and the other an
.elevation , with the expectation that one is above the ground while the
other is attached to it. I don’t expect to take Skyfield in that
direction, but we could have a separate discussion about it if we wanted
to, absent the considerations that swirl around the idea of the
“subpoint”.
>
>
> For my case, dropping "elevation" (or as I recommend, "altitude") from the
> method name would have made it more difficult to find the method I was
> looking for.
>
>
That’s an excellent point!
One nice thing about latlon is that it advertises the actual values the
user gets back; adding elevation makes that discoverable, too, for the user
browsing the API documentation.
>
>
> However, it seems strange to lump altitude (or distance) with latlon. It
> may make the most sense to provide them as separate methods.
>
>
That raises several implementation details that, alas, might not be at all
obvious from how I’m designing the API.
First, it happens that the three coordinates are most easily produced as a
single calculation — if users asked first for lat/lon, then separately for
elevation, the latitude and longitude would have to be computed all over
again (or cached somehow, which might raise even bigger complications).
Second, splitting out the elevation separately would break the pattern
that Skyfield establishes otherwise: that a complete triple of coordinates
is always the return value when doing a coordinate conversion.
Finally, as I myself had to be reminded up above, we’re in no case here
actually returning a latitude, longitude, or elevation. We’re returning a
big GeographicPosition object that, whether the user wants it or not, is a
full three-dimensional position that needs latitude, longitude, and
elevation.
I wonder if that’s too heavyweight. Maybe there should be a latlon() method
that only returns lat and lon, and skips the elevation computation? Maybe
an elevation method should exist that skips computing anything else? Users
who called both would duplicate some calculations, but we could always
warn about that in the docs.
What do y’all think? Here’s the calculation, for what it’s worth:
https:/ / github. com/ skyfielders/ python-skyfield/ blob/ 90067cc/
skyfield/ toposlib. py\#L183 (
https://github.com/skyfielders/python-skyfield/blob/90067cc3711f0c44f97871f5385fd57c92caf4f2/skyfield/toposlib.py%5C#L183
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#644 (reply in thread)
) , or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AC4ANH77P4JWGXI5EFLAJUDULKL3LANCNFSM5FPZPUDA
).
|
Beta Was this translation helpful? Give feedback.
-
All right! I’ve worked out a new proposal, which y’all can try out with:
To celebrate, this comment starts a new thread where it can be discussed. I’m interested in comments from both @bismurphy and @patrickwalton if they’re happy to provide feedback. (Oh, and, @patrickwalton — if you’ll type your replies into the little “Write a reply” box right below this thread, instead of into the bigger box at the bottom of the page, then your reply will go right into this thread instead of starting a new one. I wish GitHub Discussions were single-threaded; I sometimes write in the wrong box as well!) Here’s the new API:
My big question: if Skyfield offers these three methods, is it thereby exempted from providing a true subpoint method that would return a |
Beta Was this translation helpful? Give feedback.
-
I believe the
subpoint
function performs a somewhat counterintuitive behavior. Observe the following simple code snippet which attempts to find the subpoint of the ISS:This returns:
WGS84 latitude -9.0774 N longitude 29.3203 E elevation 423813.1 m
Note that it indicates an elevation of 423813 meters: The altitude of the ISS above the ground. I believe that the
subpoint
function ought to perform the task of identifying the subpoint of the ISS - which would be that point which is on the ground and is directly below the ISS. That position should have a distance which is approximately 6371 kilometers from the center of the earth. The current behavior is certainly useful, but it seems like it would be better performed by a function along the lines ofwgs84.lat_long_elev(position)
. Currently thesubpoint
function doesn't really do anything involving the subpoint at all: In fact, you could just as well represent a subpoint in ECEF XYZ coordinates as you could represent it in lat/long coordinates. The action of determining the subpoint is one of identifying a point below a satellite, rather than being the action of converting a position to a lat/long frame.Does anyone have any other thoughts? The current
subpoint
function doesn't exactly do what I would imagine. I was attempting to use subpoints to find the height of the geoid below the ISS, but that's not directly exposed through any function I'm aware of. Ideally I'd like to be able to runlength_of
, just as is used in the "Which geographic location is farther from Earth’s center?" example, and run that on the subpoint, but thesubpoint
function does not actually return the subpoint.Beta Was this translation helpful? Give feedback.
All reactions