Repository: graphhopper/graphhopper
Pull request #1032 (2017-04-20 05:11:59)

Correct too small weightings for CH
As discussed in #1013, setting too small weights for CH leads to weights of 0. This can lead to very weird results. Therefore, I introduced a MIN_WEIGHT, in addition to the MAX_WEIGHT. If a weight is smaller than the min weight, the weight is corrected to the min weight. In addition I added a warning message so that developers can correct their weightings more easily.

karussell: Hmmh, I do not know if this is the best solution. Detecting it via weighting.getMinWeight would be preferable as then we can throw this at start already. Will have to think about it.
boldtrn: >  Detecting it via weighting.getMinWeight would be preferable as then we can throw this at start already.

Yes makes sense as well. The problem with getMinWeight is that it depends on the distance of the edge. So we might be able to generate very small weight for edges that are very short (e.g. 1m long). The current FastestWeighting would return `1/140`, which is already kind of close to the `MIN_WEIGHT`. Maybe we should define that getMinWeight for 1m cannot be smaller than `MIN_WEIGHT`, when having CH enabled?
boldtrn: Any updates on this PR?
karussell: What if we throw an exception instead of just silently logging it?
boldtrn: > What if we throw an exception instead of just silently logging it?

Ok sure, we can do this.
boldtrn: Ok, I updated this PR to throw an exception.

Patch:
diff --git a/core/src/main/java/com/graphhopper/storage/CHGraphImpl.java b/core/src/main/java/com/graphhopper/storage/CHGraphImpl.java
index c32eb9ef2..0fca164ca 100644
--- a/core/src/main/java/com/graphhopper/storage/CHGraphImpl.java
+++ b/core/src/main/java/com/graphhopper/storage/CHGraphImpl.java
@@ -28,6 +28,8 @@ import com.graphhopper.storage.BaseGraph.CommonEdgeIterator;
 import com.graphhopper.storage.BaseGraph.EdgeIterable;
 import com.graphhopper.util.*;
 import com.graphhopper.util.shapes.BBox;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static com.graphhopper.util.Helper.nf;
 
@@ -39,10 +41,12 @@ import static com.graphhopper.util.Helper.nf;
  * @author Peter Karich
  */
 public class CHGraphImpl implements CHGraph, Storable<CHGraph> {
+    private static final Logger LOGGER = LoggerFactory.getLogger(CHGraphImpl.class);
     private static final double WEIGHT_FACTOR = 1000f;
     // 2 bits for access, for now only 32bit => not Long.MAX
     private static final long MAX_WEIGHT_LONG = (Integer.MAX_VALUE >> 2) << 2;
     private static final double MAX_WEIGHT = (Integer.MAX_VALUE >> 2) / WEIGHT_FACTOR;
+    private static final double MIN_WEIGHT = 1 / WEIGHT_FACTOR;
     final DataAccess shortcuts;
     final DataAccess nodesCH;
     final long scDirMask = PrepareEncoder.getScDirMask();
@@ -283,6 +287,13 @@ public class CHGraphImpl implements CHGraph, Storable<CHGraph> {
             throw new IllegalArgumentException("weight cannot be negative but was " + weight);
 
         long weightLong;
+
+        if (weight < MIN_WEIGHT) {
+            NodeAccess nodeAccess = getNodeAccess();
+            throw new IllegalStateException("Setting weights smaller than " + MIN_WEIGHT + " is not allowed in CHGraphImpl#setWeight. " +
+                    "You passed: " + weight + " for the edge from " + nodeAccess.getLat(edge.getBaseNode()) + "," + nodeAccess.getLon(edge.getBaseNode()) +
+                    " to " + nodeAccess.getLat(edge.getAdjNode()) + "," + nodeAccess.getLon(edge.getAdjNode()));
+        }
         if (weight > MAX_WEIGHT)
             weightLong = MAX_WEIGHT_LONG;
         else


Test patch:
diff --git a/core/src/test/java/com/graphhopper/storage/GraphHopperStorageCHTest.java b/core/src/test/java/com/graphhopper/storage/GraphHopperStorageCHTest.java
index 87d665f63..1d0b2e2de 100644
--- a/core/src/test/java/com/graphhopper/storage/GraphHopperStorageCHTest.java
+++ b/core/src/test/java/com/graphhopper/storage/GraphHopperStorageCHTest.java
@@ -195,6 +195,13 @@ public class GraphHopperStorageCHTest extends GraphHopperStorageTest {
         assertEquals(100.123, sc1.getWeight(), 1e-3);
         assertFalse(sc1.isForward(carEncoder));
         assertTrue(sc1.isBackward(carEncoder));
+
+        try{
+            // check min weight
+            sc1.setFlags(flags);
+            sc1.setWeight(1e-5);
+            fail("No exception was thrown");
+        }catch (IllegalStateException e){}
     }
 
     @Test