Repository: debrief/pepys-import

Issue:
Bug in storing zero values for fields in various importers
In various importers we have code that looks like this:

```
elevation = convert_distance(elevation_str, unit_registry.metre, None, self.errors, self.error_type)
if elevation:
    state.elevation = elevation
```

The `convert_distance` function returns False if there is an error converting the elevation string to a float. _However_, the `if elevation` statement will only be true if elevation is 'truthy' in Python. And, unfortunately in this case, a value of zero is not considered truthy. Thus, in this case elevation won't be set, and will be left at its default value of None in the database.

There are various potential ways of fixing this:

1. The quick and nasty way is to replace the if statement with `if elevation != False or elevation == 0`, but that's a fairly nasty bit of code.

2. A nicer way is to use a different 'sentinel value'. For example, we return a string of 'ERROR' when there is an error, and then check for that.

3. The nicest, but most verbose way, is to return two values from all these conversion functions: an error _and_ the actual converted value. So the code would be like:

```
elevation_valid, elevation = convert_distance(elevation_str, unit_registry.metre, None, self.errors, self.error_type)
if elevation_valid:
    state.elevation = elevation
```

Shall I go ahead and implement option 3?

Discussion:

Patch:
diff --git a/importers/e_trac_importer.py b/importers/e_trac_importer.py
index f13334ff..5cf138c7 100644
--- a/importers/e_trac_importer.py
+++ b/importers/e_trac_importer.py
@@ -115,24 +115,24 @@ class ETracImporter(Importer):
                 self.name, "location", state.location, "decimal degrees"
             )
 
-        elevation = convert_distance(
+        elevation_valid, elevation = convert_distance(
             altitude_token.text, unit_registry.metre, line_number, self.errors, self.error_type
         )
-        if elevation:
+        if elevation_valid:
             state.elevation = elevation
             altitude_token.record(self.name, "altitude", state.elevation)
 
-        heading = convert_absolute_angle(
+        heading_valid, heading = convert_absolute_angle(
             heading_token.text, line_number, self.errors, self.error_type
         )
-        if heading:
+        if heading_valid:
             state.heading = heading
             heading_token.record(self.name, "heading", heading)
 
-        speed = convert_speed(
+        speed_valid, speed = convert_speed(
             speed_token.text, unit_registry.knots, line_number, self.errors, self.error_type,
         )
-        if speed:
+        if speed_valid:
             state.speed = speed
             speed_token.record(self.name, "speed", speed)
 
diff --git a/importers/gpx_importer.py b/importers/gpx_importer.py
index b0c1344a..6403c7fd 100644
--- a/importers/gpx_importer.py
+++ b/importers/gpx_importer.py
@@ -106,29 +106,29 @@ class GPXImporter(Importer):
 
                 # Add course
                 if course_str is not None:
-                    course = convert_absolute_angle(
+                    course_valid, course = convert_absolute_angle(
                         course_str, tpt.sourceline, self.errors, self.error_type
                     )
-                    if course:
+                    if course_valid:
                         state.course = course
 
                 # Add speed (specified in metres per second in the file)
                 if speed_str is not None:
-                    speed = convert_speed(
+                    speed_valid, speed = convert_speed(
                         speed_str,
                         (unit_registry.metre / unit_registry.second),
                         None,
                         self.errors,
                         self.error_type,
                     )
-                    if speed:
+                    if speed_valid:
                         state.speed = speed
 
                 if elevation_str is not None:
-                    elevation = convert_distance(
+                    elevation_valid, elevation = convert_distance(
                         elevation_str, unit_registry.metre, None, self.errors, self.error_type
                     )
-                    if elevation:
+                    if elevation_valid:
                         state.elevation = elevation
 
     def get_child_text_if_exists(self, element, search_string):
diff --git a/importers/nmea_importer.py b/importers/nmea_importer.py
index 000700cf..f4c074e7 100644
--- a/importers/nmea_importer.py
+++ b/importers/nmea_importer.py
@@ -150,25 +150,25 @@ class NMEAImporter(Importer):
                     self.name, "location", state.location, "DMS"
                 )
 
-                heading = convert_absolute_angle(
+                heading_valid, heading = convert_absolute_angle(
                     self.heading, line_number, self.errors, self.error_type
                 )
-                if heading:
+                if heading_valid:
                     state.heading = heading
                     self.heading_token.record(self.name, "heading", heading)
 
-                speed = convert_speed(
+                speed_valid, speed = convert_speed(
                     self.speed, unit_registry.knots, line_number, self.errors, self.error_type,
                 )
-                if speed:
+                if speed_valid:
                     state.speed = speed
                     self.speed_token.record(self.name, "speed", speed)
 
                 if self.depth is not None:
-                    depth = convert_distance(
+                    depth_valid, depth = convert_distance(
                         self.depth, unit_registry.metre, line_number, self.errors, self.error_type
                     )
-                    if depth:
+                    if depth_valid:
                         state.elevation = -1 * depth
                         self.depth_token.record(self.name, "depth", depth)
 
diff --git a/importers/replay_contact_importer.py b/importers/replay_contact_importer.py
index dfbe4a1e..e8ef2df1 100644
--- a/importers/replay_contact_importer.py
+++ b/importers/replay_contact_importer.py
@@ -216,27 +216,27 @@ class ReplayContactImporter(Importer):
             if bearing_token.text.upper() == "NULL":
                 bearing = None
             else:
-                bearing = convert_absolute_angle(
+                bearing_valid, bearing = convert_absolute_angle(
                     bearing_token.text, line, self.errors, self.error_type
                 )
-                if bearing:
+                if bearing_valid:
                     bearing_token.record(self.name, "bearing", bearing, "degs")
                     contact.bearing = bearing
 
             if range_token.text.upper() != "NULL":
-                range_val = convert_distance(
+                range_valid, range_val = convert_distance(
                     range_token.text, unit_registry.yard, line, self.errors, self.error_type,
                 )
-                if range_val:
+                if range_valid:
                     range_token.record(self.name, "range", range_val)
                     contact.range = range_val
 
             if freq_token is not None:
                 if freq_token.text.upper() != "NULL":
-                    freq_val = convert_frequency(
+                    freq_valid, freq_val = convert_frequency(
                         freq_token.text, unit_registry.hertz, line, self.errors, self.error_type,
                     )
-                    if freq_val:
+                    if freq_valid:
                         freq_token.record(self.name, "frequency", freq_val)
                         contact.freq = freq_val
 
@@ -244,9 +244,9 @@ class ReplayContactImporter(Importer):
                 if ambig_bearing_token.text.upper() == "NULL":
                     ambig_bearing = 0
                 else:
-                    ambig_bearing = convert_absolute_angle(
+                    ambig_bearing_valid, ambig_bearing = convert_absolute_angle(
                         ambig_bearing_token.text, line, self.errors, self.error_type
                     )
-                    if ambig_bearing:
+                    if ambig_bearing_valid:
                         ambig_bearing_token.record(self.name, "ambig bearing", ambig_bearing)
                         contact.ambig_bearing = ambig_bearing
diff --git a/pepys_import/core/formats/rep_line.py b/pepys_import/core/formats/rep_line.py
index 47e16461..9b4f77b9 100644
--- a/pepys_import/core/formats/rep_line.py
+++ b/pepys_import/core/formats/rep_line.py
@@ -167,17 +167,19 @@ class REPLine:
             long_degrees_token, long_mins_token, long_secs_token, long_hemi_token
         ).record(self.importer_name, "longitude", self.location, "DMS")
 
-        heading = convert_absolute_angle(heading_token.text, self.line_num, errors, error_type)
-        if not heading:
+        heading_valid, heading = convert_absolute_angle(
+            heading_token.text, self.line_num, errors, error_type
+        )
+        if not heading_valid:
             return False
 
         self.heading = heading
         heading_token.record(self.importer_name, "heading", self.heading)
 
-        speed = convert_speed(
+        speed_valid, speed = convert_speed(
             speed_token.text, unit_registry.knots, self.line_num, errors, error_type
         )
-        if not speed:
+        if not speed_valid:
             return False
         self.speed = speed
         speed_token.record(self.importer_name, "speed", self.speed)
diff --git a/pepys_import/utils/unit_utils.py b/pepys_import/utils/unit_utils.py
index 5f4d5dd6..6b1c6625 100644
--- a/pepys_import/utils/unit_utils.py
+++ b/pepys_import/utils/unit_utils.py
@@ -15,7 +15,7 @@ def convert_absolute_angle(angle, line_number, errors, error_type):
     :type errors: List
     :param error_type: Type of error
     :type error_type: String
-    :return: returns the converted angle value
+    :return: Returns a tuple of a boolean is_valid flag and the converted angle value
     """
     try:
         valid_angle = float(angle)
@@ -26,12 +26,14 @@ def convert_absolute_angle(angle, line_number, errors, error_type):
                 f"Couldn't convert to a number"
             }
         )
-        return False
+        return False, None
     if valid_angle < 0:
         valid_angle += 360
     if valid_angle > 360:
         valid_angle -= 360
-    return valid_angle * unit_registry.degree
+
+    valid_angle = valid_angle * unit_registry.degree
+    return True, valid_angle
 
 
 def convert_speed(speed, units, line_number, errors, error_type):
@@ -48,7 +50,7 @@ def convert_speed(speed, units, line_number, errors, error_type):
     :type errors: List
     :param error_type: Type of error
     :type error_type: String
-    :return: return the converted speed value
+    :return: Returns a tuple of a boolean is_valid flag, and the converted speed value
     """
     try:
         valid_speed = float(speed)
@@ -59,62 +61,9 @@ def convert_speed(speed, units, line_number, errors, error_type):
                 f"Couldn't convert to a number"
             }
         )
-        return False
+        return False, None
     speed = valid_speed * units
-    return speed
-
-
-def extract_points(location):
-    """Convert decimal degrees to radians and return
-
-    :param location: A point of location
-    :type location: Location
-    """
-    return radians(location.longitude), radians(location.latitude)
-
-
-def bearing_between_two_points(first_location, second_location):
-    """
-    Calculate the bearing(angle) between two points on the earth (specified in decimal degrees).
-
-    :param first_location: First location point
-    :param second_location: Second location point
-    :return: angle between two points in degrees
-    """
-    longitude_1, latitude_1 = extract_points(first_location)
-    longitude_2, latitude_2 = extract_points(second_location)
-    diff_longitude = longitude_2 - longitude_1
-
-    y = sin(diff_longitude) * cos(latitude_2)
-    x = cos(latitude_1) * sin(latitude_2) - sin(latitude_1) * cos(latitude_2) * cos(diff_longitude)
-    bearing = degrees((atan2(y, x)))
-    bearing = (bearing + 360) % 360
-    return bearing
-
-
-def distance_between_two_points_haversine(first_location, second_location):
-    """
-    Calculate the great circle distance between two points on the earth (specified in
-    decimal degrees).
-
-    :param first_location: First location point
-    :param second_location: Second location point
-    :return: distance in kilometers
-    """
-    longitude_1, latitude_1 = extract_points(first_location)
-    longitude_2, latitude_2 = extract_points(second_location)
-
-    # haversine formula
-    diff_longitude = longitude_2 - longitude_1
-    diff_latitude = latitude_2 - latitude_1
-    a = (
-        sin(diff_latitude / 2) ** 2
-        + cos(latitude_1) * cos(latitude_2) * sin(diff_longitude / 2) ** 2
-    )
-    c = 2 * asin(sqrt(a))
-    radius = 6371  # Radius of earth in kilometers. Use 3956 for miles
-    distance = c * radius
-    return (distance * unit_registry.kilometer).to(unit_registry.meter)
+    return True, speed
 
 
 def convert_frequency(frequency, units, line_number, errors, error_type):
@@ -132,7 +81,7 @@ def convert_frequency(frequency, units, line_number, errors, error_type):
     :type errors: List
     :param error_type: Type of error
     :type error_type: String
-    :return: return the converted speed value
+    :return: Returns a tuple of a boolean is_valid flag, and the converted frequency value
     """
     try:
         valid_frequency = float(frequency)
@@ -143,9 +92,9 @@ def convert_frequency(frequency, units, line_number, errors, error_type):
                 f"Couldn't convert to a number"
             }
         )
-        return False
+        return False, None
     frequency = valid_frequency * units
-    return frequency
+    return True, frequency
 
 
 def convert_distance(distance, units, line_number, errors, error_type):
@@ -163,7 +112,7 @@ def convert_distance(distance, units, line_number, errors, error_type):
     :type errors: List
     :param error_type: Type of error
     :type error_type: String
-    :return: return the converted speed value
+    :return: Returns a tuple of a boolean is_valid flag, and the converted distance value
     """
     try:
         valid_distance = float(distance)
@@ -174,9 +123,62 @@ def convert_distance(distance, units, line_number, errors, error_type):
                 f"Couldn't convert to a number"
             }
         )
-        return False
+        return False, None
     distance = valid_distance * units
-    return distance
+    return True, distance
+
+
+def extract_points(location):
+    """Convert decimal degrees to radians and return
+
+    :param location: A point of location
+    :type location: Location
+    """
+    return radians(location.longitude), radians(location.latitude)
+
+
+def bearing_between_two_points(first_location, second_location):
+    """
+    Calculate the bearing(angle) between two points on the earth (specified in decimal degrees).
+
+    :param first_location: First location point
+    :param second_location: Second location point
+    :return: angle between two points in degrees
+    """
+    longitude_1, latitude_1 = extract_points(first_location)
+    longitude_2, latitude_2 = extract_points(second_location)
+    diff_longitude = longitude_2 - longitude_1
+
+    y = sin(diff_longitude) * cos(latitude_2)
+    x = cos(latitude_1) * sin(latitude_2) - sin(latitude_1) * cos(latitude_2) * cos(diff_longitude)
+    bearing = degrees((atan2(y, x)))
+    bearing = (bearing + 360) % 360
+    return bearing
+
+
+def distance_between_two_points_haversine(first_location, second_location):
+    """
+    Calculate the great circle distance between two points on the earth (specified in
+    decimal degrees).
+
+    :param first_location: First location point
+    :param second_location: Second location point
+    :return: distance in kilometers
+    """
+    longitude_1, latitude_1 = extract_points(first_location)
+    longitude_2, latitude_2 = extract_points(second_location)
+
+    # haversine formula
+    diff_longitude = longitude_2 - longitude_1
+    diff_latitude = latitude_2 - latitude_1
+    a = (
+        sin(diff_latitude / 2) ** 2
+        + cos(latitude_1) * cos(latitude_2) * sin(diff_longitude / 2) ** 2
+    )
+    c = 2 * asin(sqrt(a))
+    radius = 6371  # Radius of earth in kilometers. Use 3956 for miles
+    distance = c * radius
+    return (distance * unit_registry.kilometer).to(unit_registry.meter)
 
 
 def acceptable_bearing_error(bearing1, bearing2, delta):


Test patch:
diff --git a/tests/test_load_gpx.py b/tests/test_load_gpx.py
index 69734943..a1637566 100644
--- a/tests/test_load_gpx.py
+++ b/tests/test_load_gpx.py
@@ -75,6 +75,16 @@ class GPXTests(unittest.TestCase):
             )
             assert len(elev_states) == 1
 
+            # there should be a 20 points with an elevation of 0m
+            # (this proves that zero values are imported properly and not
+            # treated as errors)
+            elev_zero_states = (
+                self.store.session.query(self.store.db_classes.State)
+                .filter(self.store.db_classes.State.elevation == 0)
+                .all()
+            )
+            assert len(elev_zero_states) == 20
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/test_load_rep.py b/tests/test_load_rep.py
index cb2f9ebb..cd8d2c65 100644
--- a/tests/test_load_rep.py
+++ b/tests/test_load_rep.py
@@ -62,6 +62,16 @@ class TestLoadREP(unittest.TestCase):
 
             assert len(states_with_no_elevation) == 1
 
+            # there should be 581 points with an elevation of 0m
+            # (this proves that zero values are imported properly and not
+            # treated as errors)
+            elev_zero_states = (
+                self.store.session.query(self.store.db_classes.State)
+                .filter(self.store.db_classes.State.elevation == 0)
+                .all()
+            )
+            assert len(elev_zero_states) == 581
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/test_unit_utils.py b/tests/test_unit_utils.py
index 9a328164..a9da21bc 100644
--- a/tests/test_unit_utils.py
+++ b/tests/test_unit_utils.py
@@ -18,8 +18,9 @@ def test_convert_abs_angle_valid(input_, actual_result):
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_absolute_angle(input_, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_absolute_angle(input_, 0, errors, error_type)
 
+    assert is_valid
     assert result == actual_result * unit_registry.degree
 
 
@@ -27,9 +28,10 @@ def test_convert_abs_angle_invalid():
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_absolute_angle("blah", 0, errors, error_type)
+    is_valid, result = unit_utils.convert_absolute_angle("blah", 0, errors, error_type)
 
-    assert result is False
+    assert is_valid is False
+    assert result is None
     assert len(errors) == 1
     assert "Error in angle value" in errors[0]["test"]
 
@@ -51,8 +53,9 @@ def test_convert_speed_valid(input_, units, actual_result):
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_speed(input_, units, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_speed(input_, units, 0, errors, error_type)
 
+    assert is_valid
     assert result == actual_result
 
 
@@ -60,9 +63,10 @@ def test_convert_speed_invalid():
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_speed("blah", unit_registry.knots, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_speed("blah", unit_registry.knots, 0, errors, error_type)
 
-    assert result is False
+    assert is_valid is False
+    assert result is None
     assert len(errors) == 1
     assert "Error in speed value" in errors[0]["test"]
 
@@ -89,8 +93,9 @@ def test_convert_frequency_valid(input_, units, actual_result):
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_frequency(input_, units, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_frequency(input_, units, 0, errors, error_type)
 
+    assert is_valid
     assert result == actual_result
 
 
@@ -98,9 +103,12 @@ def test_convert_frequency_invalid():
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_frequency("blah", unit_registry.hertz, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_frequency(
+        "blah", unit_registry.hertz, 0, errors, error_type
+    )
 
-    assert result is False
+    assert is_valid is False
+    assert result is None
     assert len(errors) == 1
     assert "Error in frequency value" in errors[0]["test"]
 
@@ -116,8 +124,9 @@ def test_convert_distance_valid(input_, units, actual_result):
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_distance(input_, units, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_distance(input_, units, 0, errors, error_type)
 
+    assert is_valid
     assert result == actual_result
 
 
@@ -125,9 +134,12 @@ def test_convert_distance_invalid():
     errors = []
     error_type = "test"
 
-    result = unit_utils.convert_distance("blah", unit_registry.metre, 0, errors, error_type)
+    is_valid, result = unit_utils.convert_distance(
+        "blah", unit_registry.metre, 0, errors, error_type
+    )
 
-    assert result is False
+    assert is_valid is False
+    assert result is None
     assert len(errors) == 1
     assert "Error in distance value" in errors[0]["test"]