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

Question

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_to, self.bearing_fro, self.distance


odometer = Odometer()


class Crosstrack(object):
    def __init__(self):
        self.startpoint_lat = None
        self.startpoint_lon = None
        self.a2b_radians = None
        self.trip_distance = {}  # similar to 'odometer.distance', except A to C distance from last reset.
        self.distance = {}

    def do_crosstrack(self, current_lat, current_lon, start_lat=None, start_lon=None,
                      end_lat=None, end_lon=None, units='meters'):
        """
        return crosstrack distance from current position, and last set track or
        return crosstrack distance from current position, start, and end positions

        Formula from:
        http: // williams.best.vwh.net / avform.htm  # XTE

        """
        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.
        crosstrack_distance = {}

        try:
            if all([start_lon, start_lon, end_lat, end_lon]):  # both start and end shall start and end together.
                # A to B
                a2b_bearing, _, _ = odometer.do_to_fro_distance(start_lat, start_lon, end_lat, end_lon)
                self.a2b_radians = math.radians(a2b_bearing)
                self.startpoint_lat = start_lat
                self.startpoint_lon = start_lon
                # A to C
            if not any([start_lat, start_lon, end_lat, end_lon]):
                if not self.startpoint_lat:  # one is enough, or one is not enough, depending on perspective
                    self.startpoint_lat = default_lat  # resort to defaults
                    self.startpoint_lon = default_lon
                    self.a2b_radians = default_a2b_radians
                start_lat = self.startpoint_lat
                start_lon = self.startpoint_lon

            bearing_to, _, distance = odometer.do_to_fro_distance(start_lat, start_lon, current_lat, current_lon)
            a2c_radians = math.radians(bearing_to)

            self.trip_distance = distance  # Distance A to C in meters

            # Crosstrack calculations in radians, output as a complex numbers {'meters': (281.8893996162284+0j)
            crosstrack_distance['meters'] = (asin(sin(distance['meters']) * sin(a2c_radians - self.a2b_radians))
                                             * earth_radius)
        except Exception as error:
            print('Can\'t calculate crosstrack because: ', error)

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

            self.distance = crosstrack_distance.copy()

            # Printing for testing
            print('Positive should mean right of course, negative means left with the other choice.')
            for k, v in crosstrack_distance.items():
                print(k, ":", v.real)  # extract 'real' number

            print("Scabed on xtrk trip distance:", self.trip_distance)

            print('Real and imaginary in complex numbers:\n')

        return self.distance


crosstrack = Crosstrack()


# WIP
class BestScenario(object):
    """ Hypothetical 'if' calculations for speculation, fairy tales, and general BS
    deceptively based on a few 'real world' inputs.  Variations on a theme requiring
    current position
    current time
    current speed
    current course
    waypoint, distance and bearing
    """
    pass

    def closest_approach(self):
        """ Present course will pass X distance from waypoint,
        or course required to maintain minimum distance  (inverse crosstrack)
        current position
        waypoint
        """
        pass

    def eta(self):
        """ ETA given present course and speed
        waypoint distance and bearing
        current speed
        current course
        current time
        """
        pass

    def shortest(self):
        """ Shortest distance to waypoint given present course
        waypoint distance and bearing
        current course
        """
        pass

    def vmg(self):
        """ Velocity Made Good given present course and speed
        waypoint bearing
        current course
        current speed
        """
        pass

    def whentack(self):
        """ When to tack given present course, speed, and pre-determined angle
        waypoint bearing
        current course
        course delta required
        current speed
        """
        pass

Answer 1

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.