Add input validation warnings to Point (#272)

geopy/compat.py

@@ -36,6 +36,16 @@ def cmp(a, b):
 else:
     cmp = cmp  # builtin in py2
 
+
+if py3k:
+    from math import isfinite
+else:
+    from math import isinf, isnan
+
+    def isfinite(x):
+        return not isinf(x) and not isnan(x)
+
+
 if py3k: # pragma: no cover
     from urllib.parse import (urlencode, quote, # pylint: disable=W0611,F0401,W0611,E0611
                               urlparse, parse_qs)

geopy/point.py

@@ -5,6 +5,7 @@
 
 import collections
 import re
+import warnings
 from math import fmod
 from itertools import islice
 from geopy import util, units
@@ -15,7 +16,7 @@
     format_degrees,
     format_distance,
 )
-from geopy.compat import string_compare
+from geopy.compat import string_compare, isfinite
 
 
 POINT_PATTERN = re.compile(r"""
@@ -60,6 +61,38 @@ def _normalize_angle(x, limit):
     return modulo
 
 
+def _normalize_coordinates(latitude, longitude, altitude):
+    latitude = float(latitude or 0.0)
+    longitude = float(longitude or 0.0)
+    altitude = float(altitude or 0.0)
+
+    is_all_finite = all(isfinite(x) for x in (latitude, longitude, altitude))
+    if not is_all_finite:
+        warnings.warn('Point coordinates should be finite. NaN or inf has '
+                      'been passed as a coordinate.  This will cause a '
+                      'ValueError exception in the future versions of geopy.',
+                      UserWarning)
+
+    if abs(latitude) > 90:
+        warnings.warn('Latitude has been normalized, because its '
+                      'absolute value was more than 90. This is probably '
+                      'not what was meant, because the normalized value '
+                      'is on a different pole. If you pass coordinates as '
+                      'positional args, please make sure that the order is '
+                      '(latitude, longitude) or (y, x) in Cartesian terms.  '
+                      'This will cause a ValueError exception in the future '
+                      'versions of geopy.',
+                      UserWarning)
+        latitude = _normalize_angle(latitude, 90.0)
+
+    if abs(longitude) > 180:
+        # Longitude normalization is pretty straightforward and doesn't seem
+        # to be error-prone, so there's nothing to complain about.
+        longitude = _normalize_angle(longitude, 180.0)
+
+    return latitude, longitude, altitude
+
+
 class Point(object):
     """
     A geodetic point with latitude, longitude, and altitude.
@@ -124,12 +157,10 @@ def __new__(cls, latitude=None, longitude=None, altitude=None):
         :param float longitude: Longitude of point.
         :param float altitude: Altitude of point.
         """
-        single_arg = longitude is None and altitude is None
+        single_arg = latitude is not None and longitude is None and altitude is None
         if single_arg and not isinstance(latitude, util.NUMBER_TYPES):
             arg = latitude
-            if arg is None: # pragma: no cover
-                pass
-            elif isinstance(arg, Point):
+            if isinstance(arg, Point):
                 return cls.from_point(arg)
             elif isinstance(arg, string_compare):
                 return cls.from_string(arg)
@@ -143,15 +174,17 @@ def __new__(cls, latitude=None, longitude=None, altitude=None):
                 else:
                     return cls.from_sequence(seq)
 
-        latitude = float(latitude or 0.0)
-        if abs(latitude) > 90:
-            latitude = _normalize_angle(latitude, 90.0)
-
-        longitude = float(longitude or 0.0)
-        if abs(longitude) > 180:
-            longitude = _normalize_angle(longitude, 180.0)
+        if single_arg:
+            warnings.warn('A single number has been passed to the Point '
+                          'constructor. This is probably a mistake, because '
+                          'constructing a Point with just a latitude '
+                          'seems senseless. If this is exactly what was '
+                          'meant, then pass the zero longitude explicitly '
+                          'to get rid of this warning.',
+                          UserWarning)
 
-        altitude = float(altitude or 0.0)
+        latitude, longitude, altitude = \
+            _normalize_coordinates(latitude, longitude, altitude)
 
         self = super(Point, cls).__new__(cls)
         self.latitude = latitude
@@ -165,7 +198,8 @@ def __getitem__(self, index):
     def __setitem__(self, index, value):
         point = list(self)
         point[index] = value  # list handles slices
-        self.latitude, self.longitude, self.altitude = point
+        self.latitude, self.longitude, self.altitude = \
+            _normalize_coordinates(*point)
 
     def __iter__(self):
         return iter((self.latitude, self.longitude, self.altitude))

test/test_distance.py

@@ -7,6 +7,7 @@
 
 from nose.tools import assert_raises, assert_almost_equal # pylint: disable=E0611
 
+from geopy import distance as geopy_distance
 from geopy.point import Point
 from geopy.distance import (Distance,
                             GreatCircleDistance,
@@ -86,9 +87,14 @@ def test_should_tolerate_nans(self):
         # This is probably a bad design to silently turn NaNs into NaNs
         # instead of raising a ValueError, but this is the current behaviour,
         # hence this test.
-        self.assertTrue(math.isnan(self.cls((nan, nan), (1, 1)).kilometers))
-        self.assertTrue(math.isnan(self.cls((nan, 1), (1, nan)).kilometers))
-        self.assertTrue(math.isnan(self.cls((nan, 1), (nan, 1)).kilometers))
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            self.assertTrue(math.isnan(self.cls((nan, nan), (1, 1)).kilometers))
+            self.assertEqual(1, len(w))
+            self.assertTrue(math.isnan(self.cls((nan, 1), (1, nan)).kilometers))
+            self.assertEqual(3, len(w))  # 1 per each point tuple
+            self.assertTrue(math.isnan(self.cls((nan, 1), (nan, 1)).kilometers))
+            self.assertEqual(5, len(w))
 
     def test_should_compute_distance_for_multiple_points_pairwise(self):
         dist_total = self.cls((10, 20), (40, 60), (0, 80), (0, 10))
@@ -99,6 +105,33 @@ def test_should_compute_distance_for_multiple_points_pairwise(self):
         assert_almost_equal(dist_total.kilometers,
                             dist1.km + dist2.km + dist3.km)
 
+    def test_should_warn_when_using_single_numbers_as_points(self):
+        # Each argument is expected to be a Point. If it's not a point,
+        # it will be wrapped in Point.
+        # Point(10) equals to Point(10, 0).
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            dist1 = self.cls(10, 20)
+            self.assertEqual(2, len(w))  # 1 per each point
+            dist2 = self.cls((10, 0), (20, 0))
+            # no warnings: explicit tuples are not that suspicious
+            self.assertEqual(2, len(w))
+        assert_almost_equal(dist1.kilometers, dist2.kilometers)
+
+    def test_should_tolerate_tuples_with_textual_numbers(self):
+        dist1 = self.cls(("1", "30"), ("20", "60"))
+        dist2 = self.cls((1, 30), (20, 60))
+        assert_almost_equal(dist1.kilometers, dist2.kilometers)
+
+    def test_should_warn_for_mixed_up_lat_lon(self):
+        lat = 40
+        lon = 120  # should exceed max lat (abs 90) to cause a warning
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            # The correct order is (lat, lon).
+            self.cls((lon, lat), (lon - 10, lat))
+            self.assertEqual(2, len(w))  # 1 per each point tuple
+
 
 class CommonMathematicalOperatorCases(object):
 

test/test_point.py

@@ -6,6 +6,7 @@
 import unittest
 import math
 import sys
+import warnings
 
 from geopy.compat import u
 from geopy.point import Point
@@ -85,14 +86,30 @@ def test_point_format_altitude(self):
         self.assertEqual(point.format_decimal('m'), "41.5, 81.0, 0.0m")
 
     def test_point_from_iterable(self):
-        self.assertEqual(Point(1, 2, 3), Point([1, 2, 3]))
-        self.assertEqual(Point(1, 2, 0), Point([1, 2]))
-        self.assertEqual(Point(1, 0, 0), Point([1]))
-        self.assertEqual(Point(0, 0, 0), Point([]))
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            self.assertEqual(Point(1, 2, 3), Point([1, 2, 3]))
+            self.assertEqual(Point(1, 2, 0), Point([1, 2]))
+            self.assertEqual(0, len(w))
+            self.assertEqual(Point(1, 0, 0), Point([1]))
+            self.assertEqual(1, len(w))  # a single latitude is discouraged
+            self.assertEqual(Point(0, 0, 0), Point([]))
+            self.assertEqual(1, len(w))
 
         with self.assertRaises(ValueError):
             Point([1, 2, 3, 4])
 
+    def test_point_from_single_number(self):
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            # Point from a single number is probably a misuse,
+            # thus it's discouraged.
+            self.assertEqual((5, 0, 0), tuple(Point(5)))
+            self.assertEqual(1, len(w))
+            # But an explicit zero longitude is fine
+            self.assertEqual((5, 0, 0), tuple(Point(5, 0)))
+            self.assertEqual(1, len(w))
+
     def test_point_from_point(self):
         point = Point(self.lat, self.lon, self.alt)
         point_copy = Point(point)
@@ -105,22 +122,33 @@ def test_point_from_generator(self):
         self.assertEqual((10, 11, 12), tuple(point))
 
     def test_point_degrees_are_normalized(self):
-        point = Point(95, 185, 375)
-        self.assertEqual((-85, -175, 375), tuple(point))
-        point = Point(-95, -185, 375)
-        self.assertEqual((85, 175, 375), tuple(point))
-        point = Point(-0.0, -0.0, 375)
-        self.assertEqual((0.0, 0.0, 375.0), tuple(point))  # note that the zeros might be negative
-        # ensure that negative zeros are normalized to the positive ones
-        self.assertEqual((1.0, 1.0, 1.0), tuple(math.copysign(1.0, x) for x in point))
-        point = Point(90, 180, 375)
-        self.assertEqual((90, 180, 375), tuple(point))
-        point = Point(-90, -180, 375)
-        self.assertEqual((-90, -180, 375), tuple(point))
-        point = Point(-270, -540, 375)
-        self.assertEqual((-90, -180, 375), tuple(point))
-        point = Point(270, 540, 375)
-        self.assertEqual((-90, -180, 375), tuple(point))
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            point = Point(95, 185, 375)
+            self.assertEqual((-85, -175, 375), tuple(point))
+            self.assertEqual(1, len(w))
+
+            point = Point(-95, -185, 375)
+            self.assertEqual((85, 175, 375), tuple(point))
+            self.assertEqual(2, len(w))
+
+            point = Point(-0.0, -0.0, 375)
+            self.assertEqual((0.0, 0.0, 375.0), tuple(point))  # note that the zeros might be negative
+            # ensure that negative zeros are normalized to the positive ones
+            self.assertEqual((1.0, 1.0, 1.0), tuple(math.copysign(1.0, x) for x in point))
+            point = Point(90, 180, 375)
+            self.assertEqual((90, 180, 375), tuple(point))
+            point = Point(-90, -180, 375)
+            self.assertEqual((-90, -180, 375), tuple(point))
+            self.assertEqual(2, len(w))
+
+            point = Point(-270, -540, 375)
+            self.assertEqual((-90, -180, 375), tuple(point))
+            self.assertEqual(3, len(w))
+
+            point = Point(270, 540, 375)
+            self.assertEqual((-90, -180, 375), tuple(point))
+            self.assertEqual(4, len(w))
 
     def test_point_degrees_normalization_does_not_lose_precision(self):
         if sys.float_info.mant_dig != 53:
@@ -145,10 +173,13 @@ def test_point_degrees_normalization_does_not_lose_precision(self):
         # (180.00000000000003 + 180) fraction doesn't fit in 52 bits.
         #
         # This test ensures that such unwanted precision loss is not happening.
-        self.assertEqual(tuple(Point(90.00000000000002, 180.00000000000003)),
-                         (-89.99999999999998, -179.99999999999997, 0))
-        self.assertEqual(tuple(Point(9.000000000000002, 1.8000000000000003)),
-                         (9.000000000000002, 1.8000000000000003, 0))
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            self.assertEqual(tuple(Point(90.00000000000002, 180.00000000000003)),
+                             (-89.99999999999998, -179.99999999999997, 0))
+            self.assertEqual(tuple(Point(9.000000000000002, 1.8000000000000003)),
+                             (9.000000000000002, 1.8000000000000003, 0))
+            self.assertEqual(1, len(w))
 
     def test_unpacking(self):
         point = Point(self.lat, self.lon, self.alt)
@@ -206,6 +237,19 @@ def test_point_setitem(self):
         self.assertEqual(point.longitude, self.lon)
         self.assertEqual(point.altitude, self.alt)
 
+    def test_point_setitem_normalization(self):
+        point = Point()
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            point[0] = 100
+            point[1] = 200
+            # Please note that attribute assignments are not normalized.
+            # Only __setitem__ assignments are.
+            self.assertEqual((-80, -160, 0), tuple(point))
+            self.assertEqual(1, len(w))  # for latitude
+            point[1] = float("nan")
+            self.assertEqual(2, len(w))
+
     def test_point_assign_coordinates(self):
         point = Point(self.lat + 10, self.lon + 10, self.alt + 10)
         point.latitude = self.lat