Repository: graphhopper/graphhopper
Pull request #1518 (2019-01-14 12:03:13)

Removes dir parameter from PrepareContractionHierarchies constructor.
`AbstractNodeContractor` uses a `DataAccess` to keep track of originalEdges that are used for node priority calculation during CH preprocessing. originalEdges are an implementation detail of the CH preprocessing and are only needed temporarily and never get flushed to disk. Therefore I think it is better to create the `DataAccess` inside `AbstractNodeContractor` as it is done here. This allows removing the dir parameter from the `PrepareContractionHierarchies` and `NodeContractor` constructors and thus decouples the algorithm code from the storage. In a next step I think it would be quite easy to replace `GraphHopperStorage` with the `Graph` interface such that it will be possible to run CH with any graph implementation implementing the `(CH)Graph` interface.



Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/ch/AbstractNodeContractor.java b/core/src/main/java/com/graphhopper/routing/ch/AbstractNodeContractor.java
index 9ce66debd..3dafe8034 100644
--- a/core/src/main/java/com/graphhopper/routing/ch/AbstractNodeContractor.java
+++ b/core/src/main/java/com/graphhopper/routing/ch/AbstractNodeContractor.java
@@ -20,12 +20,8 @@ package com.graphhopper.routing.ch;
 
 import com.graphhopper.routing.util.DefaultEdgeFilter;
 import com.graphhopper.routing.util.FlagEncoder;
-import com.graphhopper.routing.weighting.AbstractWeighting;
 import com.graphhopper.routing.weighting.Weighting;
-import com.graphhopper.storage.CHGraph;
-import com.graphhopper.storage.DataAccess;
-import com.graphhopper.storage.Directory;
-import com.graphhopper.storage.GraphHopperStorage;
+import com.graphhopper.storage.*;
 import com.graphhopper.util.CHEdgeExplorer;
 
 abstract class AbstractNodeContractor implements NodeContractor {
@@ -38,11 +34,11 @@ abstract class AbstractNodeContractor implements NodeContractor {
     int maxLevel;
     private int maxEdgesCount;
 
-    public AbstractNodeContractor(Directory dir, GraphHopperStorage ghStorage, CHGraph prepareGraph, Weighting weighting) {
+    public AbstractNodeContractor(GraphHopperStorage ghStorage, CHGraph prepareGraph, Weighting weighting) {
         this.ghStorage = ghStorage;
         this.prepareGraph = prepareGraph;
         this.encoder = weighting.getFlagEncoder();
-        originalEdges = dir.find("original_edges_" + AbstractWeighting.weightingToFileName(weighting));
+        originalEdges = new GHDirectory("", DAType.RAM_INT).find("");
         originalEdges.create(1000);
     }
 
diff --git a/core/src/main/java/com/graphhopper/routing/ch/CHAlgoFactoryDecorator.java b/core/src/main/java/com/graphhopper/routing/ch/CHAlgoFactoryDecorator.java
index 191841cca..0f3e3c034 100644
--- a/core/src/main/java/com/graphhopper/routing/ch/CHAlgoFactoryDecorator.java
+++ b/core/src/main/java/com/graphhopper/routing/ch/CHAlgoFactoryDecorator.java
@@ -264,7 +264,7 @@ public class CHAlgoFactoryDecorator implements RoutingAlgorithmFactoryDecorator
 
         for (Weighting weighting : getWeightings()) {
             PrepareContractionHierarchies tmpPrepareCH = new PrepareContractionHierarchies(
-                    new GHDirectory("", DAType.RAM_INT), ghStorage, ghStorage.getGraph(CHGraph.class, weighting), traversalMode);
+                    ghStorage, ghStorage.getGraph(CHGraph.class, weighting), traversalMode);
             tmpPrepareCH.setParams(pMap);
             addPreparation(tmpPrepareCH);
         }
diff --git a/core/src/main/java/com/graphhopper/routing/ch/NodeBasedNodeContractor.java b/core/src/main/java/com/graphhopper/routing/ch/NodeBasedNodeContractor.java
index c37e144a0..c77f4b2bb 100644
--- a/core/src/main/java/com/graphhopper/routing/ch/NodeBasedNodeContractor.java
+++ b/core/src/main/java/com/graphhopper/routing/ch/NodeBasedNodeContractor.java
@@ -47,8 +47,8 @@ class NodeBasedNodeContractor extends AbstractNodeContractor {
     // each edge can exist in both directions
     private double meanDegree;
 
-    NodeBasedNodeContractor(Directory dir, GraphHopperStorage ghStorage, CHGraph prepareGraph, Weighting weighting, PMap pMap) {
-        super(dir, ghStorage, prepareGraph, weighting);
+    NodeBasedNodeContractor(GraphHopperStorage ghStorage, CHGraph prepareGraph, Weighting weighting, PMap pMap) {
+        super(ghStorage, prepareGraph, weighting);
         this.prepareWeighting = new PreparationWeighting(weighting);
         extractParams(pMap);
     }
diff --git a/core/src/main/java/com/graphhopper/routing/ch/PrepareContractionHierarchies.java b/core/src/main/java/com/graphhopper/routing/ch/PrepareContractionHierarchies.java
index 29226da00..f511948eb 100644
--- a/core/src/main/java/com/graphhopper/routing/ch/PrepareContractionHierarchies.java
+++ b/core/src/main/java/com/graphhopper/routing/ch/PrepareContractionHierarchies.java
@@ -49,7 +49,6 @@ import static com.graphhopper.util.Parameters.Algorithms.DIJKSTRA_BI;
  */
 public class PrepareContractionHierarchies extends AbstractAlgoPreparation implements RoutingAlgorithmFactory {
     private final Logger logger = LoggerFactory.getLogger(getClass());
-    private final Directory dir;
     private final PreparationWeighting prepareWeighting;
     private final Weighting weighting;
     private final TraversalMode traversalMode;
@@ -68,13 +67,12 @@ public class PrepareContractionHierarchies extends AbstractAlgoPreparation imple
     private int maxLevel;
     // nodes with highest priority come last
     private GHTreeMapComposed sortedNodes;
-    private float oldPriorities[];
+    private float[] oldPriorities;
     private PMap pMap = new PMap();
     private int initSize;
     private int checkCounter;
 
-    public PrepareContractionHierarchies(Directory dir, GraphHopperStorage ghStorage, CHGraph chGraph, TraversalMode traversalMode) {
-        this.dir = dir;
+    public PrepareContractionHierarchies(GraphHopperStorage ghStorage, CHGraph chGraph, TraversalMode traversalMode) {
         this.ghStorage = ghStorage;
         this.prepareGraph = (CHGraphImpl) chGraph;
         this.traversalMode = traversalMode;
@@ -159,7 +157,7 @@ public class PrepareContractionHierarchies extends AbstractAlgoPreparation imple
         //   but we need the additional oldPriorities array to keep the old value which is necessary for the update method
         sortedNodes = new GHTreeMapComposed();
         oldPriorities = new float[prepareGraph.getNodes()];
-        nodeContractor = new NodeBasedNodeContractor(dir, ghStorage, prepareGraph, weighting, pMap);
+        nodeContractor = new NodeBasedNodeContractor(ghStorage, prepareGraph, weighting, pMap);
         nodeContractor.initFromGraph();
     }
 


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/DijkstraBidirectionCHTest.java b/core/src/test/java/com/graphhopper/routing/DijkstraBidirectionCHTest.java
index d8bd400fe..a32e06184 100644
--- a/core/src/test/java/com/graphhopper/routing/DijkstraBidirectionCHTest.java
+++ b/core/src/test/java/com/graphhopper/routing/DijkstraBidirectionCHTest.java
@@ -57,7 +57,7 @@ public class DijkstraBidirectionCHTest extends AbstractRoutingAlgorithmTester {
 
     @Override
     public RoutingAlgorithmFactory createFactory(GraphHopperStorage ghStorage, AlgorithmOptions opts) {
-        PrepareContractionHierarchies ch = new PrepareContractionHierarchies(new GHDirectory("", DAType.RAM_INT),
+        PrepareContractionHierarchies ch = new PrepareContractionHierarchies(
                 ghStorage, getGraph(ghStorage, opts.getWeighting()), TraversalMode.NODE_BASED);
         ch.doWork();
         return ch;
@@ -104,7 +104,7 @@ public class DijkstraBidirectionCHTest extends AbstractRoutingAlgorithmTester {
         g2.setLevel(0, 7);
 
         AlgorithmOptions opts = new AlgorithmOptions(Parameters.Algorithms.DIJKSTRA_BI, weighting);
-        Path p = new PrepareContractionHierarchies(new GHDirectory("", DAType.RAM_INT),
+        Path p = new PrepareContractionHierarchies(
                 ghStorage, g2, TraversalMode.NODE_BASED).
                 createAlgo(g2, opts).calcPath(0, 7);
 
@@ -260,7 +260,7 @@ public class DijkstraBidirectionCHTest extends AbstractRoutingAlgorithmTester {
     }
 
     private RoutingAlgorithm createCHAlgo(GraphHopperStorage graph, CHGraph chGraph, boolean withSOD, AlgorithmOptions algorithmOptions) {
-        PrepareContractionHierarchies ch = new PrepareContractionHierarchies(new GHDirectory("", DAType.RAM_INT),
+        PrepareContractionHierarchies ch = new PrepareContractionHierarchies(
                 graph, chGraph, TraversalMode.NODE_BASED);
         if (!withSOD) {
             algorithmOptions.getHints().put("stall_on_demand", false);
diff --git a/core/src/test/java/com/graphhopper/routing/ch/NodeBasedNodeContractorTest.java b/core/src/test/java/com/graphhopper/routing/ch/NodeBasedNodeContractorTest.java
index 6f67fbd5b..15cfef508 100644
--- a/core/src/test/java/com/graphhopper/routing/ch/NodeBasedNodeContractorTest.java
+++ b/core/src/test/java/com/graphhopper/routing/ch/NodeBasedNodeContractorTest.java
@@ -27,7 +27,6 @@ import com.graphhopper.util.CHEdgeIteratorState;
 import com.graphhopper.util.EdgeIterator;
 import com.graphhopper.util.EdgeIteratorState;
 import com.graphhopper.util.PMap;
-import org.junit.Before;
 import org.junit.Test;
 
 import java.util.Arrays;
@@ -44,15 +43,9 @@ public class NodeBasedNodeContractorTest {
     private final GraphHopperStorage graph = new GraphBuilder(encodingManager).setCHGraph(weighting).create();
     private final CHGraph lg = graph.getGraph(CHGraph.class);
     private final TraversalMode traversalMode = TraversalMode.NODE_BASED;
-    private Directory dir;
-
-    @Before
-    public void setUp() {
-        dir = new GHDirectory("", DAType.RAM_INT);
-    }
 
     private NodeContractor createNodeContractor() {
-        NodeContractor nodeContractor = new NodeBasedNodeContractor(dir, graph, lg, weighting, new PMap());
+        NodeContractor nodeContractor = new NodeBasedNodeContractor(graph, lg, weighting, new PMap());
         nodeContractor.initFromGraph();
         nodeContractor.prepareContraction();
         return nodeContractor;
diff --git a/core/src/test/java/com/graphhopper/routing/ch/PrepareContractionHierarchiesTest.java b/core/src/test/java/com/graphhopper/routing/ch/PrepareContractionHierarchiesTest.java
index 4fe487a03..300801254 100644
--- a/core/src/test/java/com/graphhopper/routing/ch/PrepareContractionHierarchiesTest.java
+++ b/core/src/test/java/com/graphhopper/routing/ch/PrepareContractionHierarchiesTest.java
@@ -154,7 +154,7 @@ public class PrepareContractionHierarchiesTest {
     public void testReturnsCorrectWeighting() {
         GraphHopperStorage g = createGHStorage();
         CHGraph lg = g.getGraph(CHGraph.class);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         assertSame(weighting, prepare.getWeighting());
     }
     
@@ -163,7 +163,7 @@ public class PrepareContractionHierarchiesTest {
         GraphHopperStorage g = createExampleGraph();
         CHGraph lg = g.getGraph(CHGraph.class);
         int old = lg.getAllEdges().length();
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(old + 2, lg.getAllEdges().length());
     }
@@ -174,7 +174,7 @@ public class PrepareContractionHierarchiesTest {
         CHGraph lg = g.getGraph(CHGraph.class);
         initShortcutsGraph(lg);
         int oldCount = g.getAllEdges().length();
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(oldCount, g.getAllEdges().length());
         assertEquals(oldCount + 7, lg.getAllEdges().length());
@@ -193,7 +193,7 @@ public class PrepareContractionHierarchiesTest {
         g.freeze();
         int oldCount = GHUtility.count(lg.getAllEdges());
         assertEquals(6, oldCount);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(2, prepare.getShortcuts());
         assertEquals(oldCount + 2, GHUtility.count(lg.getAllEdges()));
@@ -210,7 +210,7 @@ public class PrepareContractionHierarchiesTest {
         initDirected2(g);
         int oldCount = GHUtility.count(g.getAllEdges());
         assertEquals(19, oldCount);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         // PrepareTowerNodesShortcutsTest.printEdges(g);
         assertEquals(oldCount, g.getAllEdges().length());
@@ -283,7 +283,7 @@ public class PrepareContractionHierarchiesTest {
         CHGraph lg = g.getGraph(CHGraph.class);
         initRoundaboutGraph(g);
         int oldCount = g.getAllEdges().length();
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(oldCount, g.getAllEdges().length());
         assertEquals(oldCount + 23, lg.getAllEdges().length());
@@ -339,10 +339,10 @@ public class PrepareContractionHierarchiesTest {
 
     @Test
     public void testUnpackingOrder() {
-        GraphHopperStorage ghStorage = createGHStorage();
-        CHGraph lg = ghStorage.getGraph(CHGraph.class);
-        initUnpackingGraph(ghStorage, lg, weighting);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, ghStorage, lg, tMode);
+        GraphHopperStorage g = createGHStorage();
+        CHGraph lg = g.getGraph(CHGraph.class);
+        initUnpackingGraph(g, lg, weighting);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         RoutingAlgorithm algo = prepare.createAlgo(lg, new AlgorithmOptions(DIJKSTRA_BI, weighting, tMode));
         Path p = algo.calcPath(10, 6);
         assertEquals(7, p.getDistance(), 1e-5);
@@ -351,12 +351,12 @@ public class PrepareContractionHierarchiesTest {
 
     @Test
     public void testUnpackingOrder_Fastest() {
-        GraphHopperStorage ghStorage = createGHStorage();
-        CHGraph lg = ghStorage.getGraph(CHGraph.class);
+        GraphHopperStorage g = createGHStorage();
+        CHGraph lg = g.getGraph(CHGraph.class);
         Weighting w = new FastestWeighting(carEncoder);
-        initUnpackingGraph(ghStorage, lg, w);
+        initUnpackingGraph(g, lg, w);
 
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, ghStorage, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         RoutingAlgorithm algo = prepare.createAlgo(lg, new AlgorithmOptions(DIJKSTRA_BI, weighting, tMode));
         Path p = algo.calcPath(10, 6);
         assertEquals(7, p.getDistance(), 1e-1);
@@ -374,7 +374,7 @@ public class PrepareContractionHierarchiesTest {
         g.edge(0, 1, 4, true);
         g.edge(0, 2, 10, true);
         g.edge(0, 3, 10, true);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(0, prepare.getShortcuts());
     }
@@ -398,7 +398,7 @@ public class PrepareContractionHierarchiesTest {
         g.edge(3, 4, 1, true);
         g.edge(6, 3, 1, true);
 
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, g, lg, tMode);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg);
         prepare.doWork();
         assertEquals(2, prepare.getShortcuts());
     }
@@ -486,9 +486,9 @@ public class PrepareContractionHierarchiesTest {
         checkPath(ghStorage, bikeWeighting, 9, 5, IntArrayList.from(3, 10, 14, 16, 13, 12));
     }
 
-    void checkPath(GraphHopperStorage ghStorage, Weighting w, int expShortcuts, double expDistance, IntIndexedContainer expNodes) {
-        CHGraph lg = ghStorage.getGraph(CHGraph.class, w);
-        PrepareContractionHierarchies prepare = new PrepareContractionHierarchies(dir, ghStorage, lg, tMode);
+    void checkPath(GraphHopperStorage g, Weighting w, int expShortcuts, double expDistance, IntIndexedContainer expNodes) {
+        CHGraph lg = g.getGraph(CHGraph.class, w);
+        PrepareContractionHierarchies prepare = createPrepareContractionHierarchies(g, lg, w);
         prepare.doWork();
         assertEquals(w.toString(), expShortcuts, prepare.getShortcuts());
         RoutingAlgorithm algo = prepare.createAlgo(lg, new AlgorithmOptions(DIJKSTRA_BI, w, tMode));
@@ -497,4 +497,13 @@ public class PrepareContractionHierarchiesTest {
         assertEquals(w.toString(), expNodes, p.calcNodes());
     }
 
+    private PrepareContractionHierarchies createPrepareContractionHierarchies(GraphHopperStorage g, CHGraph lg) {
+        return createPrepareContractionHierarchies(g, lg, weighting);
+    }
+
+    private PrepareContractionHierarchies createPrepareContractionHierarchies(GraphHopperStorage g, CHGraph lg, Weighting w) {
+        g.freeze();
+        return new PrepareContractionHierarchies(g, lg, tMode);
+    }
+
 }
diff --git a/reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java b/reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java
index ef9655c33..b498d87d5 100644
--- a/reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java
+++ b/reader-osm/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java
@@ -886,8 +886,8 @@ public class GraphHopperOSMTest {
         GraphHopperStorage storage = new GraphHopperStorage(Arrays.asList(fwSimpleTruck, fwTruck), ramDir, em, false, new GraphExtension.NoOpExtension());
         decorator.addWeighting(fwSimpleTruck);
         decorator.addWeighting(fwTruck);
-        decorator.addPreparation(new PrepareContractionHierarchies(ramDir, storage, storage.getGraph(CHGraph.class, fwSimpleTruck), TraversalMode.NODE_BASED));
-        decorator.addPreparation(new PrepareContractionHierarchies(ramDir, storage, storage.getGraph(CHGraph.class, fwTruck), TraversalMode.NODE_BASED));
+        decorator.addPreparation(new PrepareContractionHierarchies(storage, storage.getGraph(CHGraph.class, fwSimpleTruck), TraversalMode.NODE_BASED));
+        decorator.addPreparation(new PrepareContractionHierarchies(storage, storage.getGraph(CHGraph.class, fwTruck), TraversalMode.NODE_BASED));
 
         HintsMap wMap = new HintsMap("fastest");
         wMap.put("vehicle", "truck");
@@ -899,7 +899,7 @@ public class GraphHopperOSMTest {
         decorator.addWeighting(fwTruck);
         decorator.addWeighting(fwSimpleTruck);
         try {
-            decorator.addPreparation(new PrepareContractionHierarchies(ramDir, storage, storage.getGraph(CHGraph.class, fwSimpleTruck), TraversalMode.NODE_BASED));
+            decorator.addPreparation(new PrepareContractionHierarchies(storage, storage.getGraph(CHGraph.class, fwSimpleTruck), TraversalMode.NODE_BASED));
             fail();
         } catch (Exception ex) {
         }