Latitude/longitude waypoint distance calculations, crosstrack, VMG, ETA, etc

I'm in the process of a complete re-write of a stagnant and ill-conceived project. I would like some feedback on the following Python 2.7-3.4 code, before I go any deeper into it.

I would hope it is self-explanatory, but that is always the illusion when you stare at something long enough. The other dilemma is glossing over errors and squirrelly logic.

```
#!/usr/bin/env python
# -- coding: utf-8 --
""" waypoint.py
A collection of waypoint and distance functions for the NxGPS Project:
Getting the most out of a $30 gps, for Navigatrix (http://navigatrix.net)
NxGPS project repository is (https://github.com/wadda/NxGPS)
"""
from __future__ import print_function
from pyproj import Geod # sudo pip3 install pyproj
from cmath import asin, sin
import math
# from math import radians, degrees
# import gps3

conversion = {'nautical': 1852.0, 'imperial': 1609.344, 'metric': 1000.0, 'meters': 1.0}

class Odometer(object):
"""Odometer is a wrapper around pyproj.Geod distance calculations"""

def __init__(self):
self.bearing_to = None
self.bearing_fro = None
self.distance = {}

def do_to_fro_distance(self, lat1, lon1, lat2, lon2, units='meters'):
"""lat lon from point A and point B, returns True North
bearings 'to', A-B(1-2), and 'fro', B-A (2-1) and distance in always
fashionable meters with optional nautical, imperial, or (kilo)metric"""
bearing_to = bearing_fro = self.distance['meters'] = None
try:
geoid = Geod(ellps='WGS84')
bearing_to, bearing_fro, self.distance['meters'] = geoid.inv(lon1, lat1, lon2, lat2)

except Exception as error:
print("Can't calculate to/fro because:", error)

finally:
if units not in 'meters':
self.distance[units] = self.distance['meters'] / conversion[units]

self.bearing_to = bearing_to % 360
self.bearing_fro = bearing_fro % 360

return self.bearing_

It's not really clear why Odometer is a class. You only ever create one instance of it, and don't actually need any of the instance attributes. The only thing you could sensibly store as a class/instance attribute, the Geod instance, you actually recreate on every call.

Also, the error handling seems a bit odd - why return anything if you haven't actually been able to calculate a result? When you get to the finally block, you end up trying to do e.g. self.bearing_to = None % 360, which will just cause another error. You could either return None, as I do below, or pass the error (or your own error) up to the calling function to deal with.

It would be good to be more specific than Exception, too (I don't know what errors inv could throw, but you should find out and check specifically for them - see e.g. "the evils of except").

I would write this as a simple function:

CONVERSION = {  # note UPPERCASE_WITH_UNDERSCORES for constants
    'imperial': 1609.344, 
    'metric': 1000.0,
    'nautical': 1852.0,
}  # no need for 'meters'

GEOID = Geod(ellps='WGS84')

def odometer(start, end, units='meters'):
    """Calculate bearings and distance between start and end.

    Arguments:
      start (tuple of float): latitude and longitude of starting position
      end (tuple of float): latitude and longitude of ending position
      units (str): the units to output distance (must be one of 'meters',
        'nautical' (miles), 'imperial' (miles) or 'metric' (km)). Defaults
        to 'meters'.

    Returns:
      float: True North bearing start -> end
      float: True North bearing end -> start
      float: distance start  end

    Raises:
      KeyError: if units isn't  'meters' or in CONVERSION

    """
    try:
        bearing_to, bearing_fro, distance = geoid.inv(*start+end)
    except Exception as error:  # try to be more specific here!
        print("Can't calculate to/fro because:", error)
        return None  # don't return anything if calculation fails
    # no 'finally' - if the calculation fails, we're done here! At most, use 'else'
    if units != 'meters':
        distance /= CONVERSION[units]  # in-place operators to simplify code
    bearing_to %= 360
    bearing_fro %= 360
    return bearing_to, bearing_fro, distance

I have simplified the arguments by taking two two-tuples (lat, long), then unpacking them to goeid.inv (see What does ** (double star) and * (star) do for Python parameters? if this syntax is unfamiliar). Alternatively, you could use a collections.namedtuple (e.g. Coord = namedtuple("Coord", "lat lon")); this would allow you to access .lat and .lon attributes while still allowing tuple unpacking.

I have also provided a more informative docstring (this is the Google style, which I like, but others are available). Using formatted docstrings like this you can then automatically generate API documentation using tools like Sphinx.

The same is true for Crosstrack. Additionally, even if you were going to keep it in a class, it redefines constants each time do_crosstrack is called; these should be class attributes instead:

class Crosstrack(object):

    DEFAULT_LAT = -15.560615  # Apataki Carenage
    DEFAULT_LON = -146.241122  # Apataki Carenage
    DEFAULT_A2B_RADIANS = 4.2538533202126025  # bearing in radians Apataki to Kaputar
    EARTH_RADIUS = 6371009.0  # meters...It's a mean radius, but nice enough.

    def __init__(self):
        ...

But, again, this would be much simpler as a standalone function.

You also have a subtle bug; note that e.g.:

all([start_lon, start_lon, end_lat, end_lon]):

will be False not just if any are None, but if any are zero (an edge case, perhaps, but one you should consider). Per the style guide, you should test for None by identity, using is:

all(coord is not None for coord in [start_lon, ...]):

For "work in progress" methods/functions, I would generally raise NotImplementedError rather than pass, so it reminds me I still need to write it if I accidentally call it before implementation.

StackExchange Code Review Q#80727, answer score: 7