Repository: graphhopper/graphhopper (java)

Issue:
average_speed was infinite error in PathDetail calculation
I am running into this error:

https://github.com/graphhopper/graphhopper/blob/e72c67d7fdad39c5cd1b3ecaeb91ec65ae5d9eb2/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java#L37-L41

I think the error is thrown whenever the edge distance is below a certain threshold (because then `calcMillis` will return zero). What mechanism is supposed to prevent this error ? We'd have to exclude edges below a certain distance or something (which as far as I know we do not) ?.

And another question: Why are we calculating speed=distance/time here ? Can we not just use the `averageSpeedEnc` to obtain the speed directly from the edge ?

Pull request:
AverageSpeedDetails: avoid exception for short edges
fixes #1848

More or less copied your fix @easbar and added a test for it :)

Patch:
diff --git a/core/src/main/java/com/graphhopper/storage/BaseGraph.java b/core/src/main/java/com/graphhopper/storage/BaseGraph.java
index 3d10d8d5c..ba958b1f9 100644
--- a/core/src/main/java/com/graphhopper/storage/BaseGraph.java
+++ b/core/src/main/java/com/graphhopper/storage/BaseGraph.java
@@ -50,8 +50,9 @@ import static com.graphhopper.util.Helper.nf;
  * loadExisting, (4) usage, (5) flush, (6) close
  */
 class BaseGraph implements Graph {
-    // currently distances are stored as 4 byte integers. using a conversion factor of 1000 the minimum distance
-    // that is not considered zero is 0.0005m (=0.5mm) and the maximum distance per edge is about 2.147.483m=2147km
+    // Currently distances are stored as 4 byte integers. using a conversion factor of 1000 the minimum distance
+    // that is not considered zero is 0.0005m (=0.5mm) and the maximum distance per edge is about 2.147.483m=2147km.
+    // See OSMReader.addEdge and #1871.
     private static final double INT_DIST_FACTOR = 1000d;
     static double MAX_DIST = Integer.MAX_VALUE / INT_DIST_FACTOR;
 
diff --git a/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java b/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
index 105600385..81dbfbd70 100644
--- a/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
+++ b/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
@@ -10,7 +10,7 @@ public class AverageSpeedDetails extends AbstractPathDetailsBuilder {
 
     private final Weighting weighting;
     private final double precision;
-    private double decimalValue = -1;
+    private Double decimalValue;
     // will include the turn time penalty
     private int prevEdgeId = -1;
 
@@ -35,12 +35,21 @@ public class AverageSpeedDetails extends AbstractPathDetailsBuilder {
 
     @Override
     public boolean isEdgeDifferentToLastEdge(EdgeIteratorState edge) {
-        double tmpVal = edge.getDistance() / GHUtility.calcMillisWithTurnMillis(weighting, edge, false, prevEdgeId) * 3600;
-        if (Double.isInfinite(tmpVal))
-            throw new IllegalStateException("average_speed was infinite for " + edge.fetchWayGeometry(3));
+        // for very short edges we might not be able calculate a proper value for speed. dividing by calcMillis can
+        // even lead to speed=Infinity -> just ignore these cases here, see #1848
+        final double distance = edge.getDistance();
+        if (distance < 0.1) {
+            if (decimalValue != null)
+                return false;
+
+            // in case this is the first edge we have to return some value
+            decimalValue = null;
+            return true;
+        }
 
+        double tmpVal = distance / GHUtility.calcMillisWithTurnMillis(weighting, edge, false, prevEdgeId) * 3600;
         prevEdgeId = edge.getEdge();
-        if (Math.abs(tmpVal - decimalValue) >= precision) {
+        if (decimalValue == null || Math.abs(tmpVal - decimalValue) >= precision) {
             this.decimalValue = Math.round(tmpVal / precision) * precision;
             return true;
         }
diff --git a/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java b/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java
index 57c11ce34..994e1320d 100644
--- a/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java
+++ b/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java
@@ -698,11 +698,11 @@ public class OSMReader implements DataReader, TurnCostParser.ExternalInternalMap
                     pillarNodes.add(lat, lon);
             }
         }
-        if (towerNodeDistance < 0.0001) {
+        if (towerNodeDistance < 0.001) {
             // As investigation shows often two paths should have crossed via one identical point 
             // but end up in two very close points.
             zeroCounter++;
-            towerNodeDistance = 0.0001;
+            towerNodeDistance = 0.001;
         }
 
         double maxDistance = (Integer.MAX_VALUE - 1) / 1000d;


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/PathTest.java b/core/src/test/java/com/graphhopper/routing/PathTest.java
index 422a6f77d..51fb890ae 100644
--- a/core/src/test/java/com/graphhopper/routing/PathTest.java
+++ b/core/src/test/java/com/graphhopper/routing/PathTest.java
@@ -316,6 +316,28 @@ public class PathTest {
         assertEquals(4, averageSpeedDetails.get(3).getLast());
     }
 
+    @Test
+    public void testCalcAverageSpeedDetailsWithShortDistances_issue1848() {
+        ShortestWeighting weighting = new ShortestWeighting(encoder);
+        Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 6);
+        assertTrue(p.isFound());
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
+        assertTrue(details.size() == 1);
+        List<PathDetail> averageSpeedDetails = details.get(AVERAGE_SPEED);
+        assertEquals(4, averageSpeedDetails.size());
+
+        // reverse path includes 'null' value as first
+        p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(6, 1);
+        assertTrue(p.isFound());
+        details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
+        assertTrue(details.size() == 1);
+        averageSpeedDetails = details.get(AVERAGE_SPEED);
+        assertEquals(5, averageSpeedDetails.size());
+        assertNull(averageSpeedDetails.get(0).getValue());
+    }
+
     @Test
     public void testCalcStreetNameDetails() {
         ShortestWeighting weighting = new ShortestWeighting(encoder);
@@ -924,6 +946,7 @@ public class PathTest {
         na.setNode(3, 52.514, 13.350);
         na.setNode(4, 52.515, 13.349);
         na.setNode(5, 52.516, 13.3452);
+        na.setNode(6, 52.516, 13.344);
 
         ReaderWay w = new ReaderWay(1);
         w.setTag("highway", "tertiary");
@@ -946,6 +969,9 @@ public class PathTest {
         tmpEdge = g.edge(3, 4, 10, true).setName("3-4");
         tmpEdge.setFlags(carManager.handleWayTags(w, map, relFlags));
 
+        tmpEdge = g.edge(5, 6, 0.01, true).setName("3-4");
+        tmpEdge.setFlags(carManager.handleWayTags(w, map, relFlags));
+
         return g;
     }