graphhopper / map-matching

The map matching functionality is now located in the main repository https://github.com/graphhopper/graphhopper#map-matching
https://www.graphhopper.com/open-source/
Apache License 2.0
785 stars 273 forks source link

Upgrade GH core to 1.0-pre4 #161

Closed karussell closed 5 years ago

karussell commented 5 years ago

There is a huge delay for MapMatchingTest when switching from 1.0-pre3 to pre4

There seems to be a problem in MapMatching.createVirtualEdgesMap. Is this due to https://github.com/graphhopper/graphhopper/pull/1751 ? Maybe traverseToClosestRealAdj is too slow now or some infinite/recursive loop without end?

easbar commented 5 years ago

There seems to be a problem

Is there (only) a performance problem or also some problem with correctness ?

karussell commented 5 years ago

It just does not finish. So either very slow or some infinite loop.

karussell commented 5 years ago

It could be traverseToClosestRealAdj that is now somehow stuck in infinite recursion

easbar commented 5 years ago

I'll have a look.

easbar commented 5 years ago

Yes there appears infinite recursion in traverseToClosestRealAdj and yes this is caused by graphhopper/graphhopper#1751 . But rather than saying this got broken in graphhopper/graphhopper#1751 I would argue the pattern used in this method is just not supported by our (graph) API for traversing the graph. This only worked prior to graphhopper/graphhopper#1751 because of the way the QueryGraph edge iterator was implemented, but with graphhopper/graphhopper#1751 this does not work anymore and it never worked and does not work for the (non-query/standard) graph. Take a look at this example using a plain GraphHopperStorage:

 @Test
    public void recursionDoesNotWorkForEdgeExplorers() {
        //
        // 4-0-1-2
        //     |
        //     3
        EncodingManager em = EncodingManager.create("car");
        FlagEncoder encoder = em.fetchEdgeEncoders().get(0);
        GraphHopperStorage graph = new GraphHopperStorage(new RAMDirectory(),
                em, false, new GraphExtension.NoOpExtension()).create(100);
        graph.edge(0, 4, 10, false);
        graph.edge(0, 1, 10, false);
        graph.edge(1, 2, 10, false);
        graph.edge(1, 3, 10, false);
        EdgeExplorer explorer = graph.createEdgeExplorer(DefaultEdgeFilter.outEdges(encoder));
        // iter0 iterates edges at node 0
        EdgeIterator iter0 = explorer.setBaseNode(0);
        iter0.next();
        assertEquals("1 0-1", iter0.toString());
        // we are not done iterating all edges at 0, but now we create 'another' (NOT) iterator that iterates edges at node 1
        EdgeIterator iter1 = explorer.setBaseNode(iter0.getAdjNode());
        iter1.next();
        // cool!
        assertEquals("3 1-3", iter1.toString());
        // ok now lets get back to iter0 again and retrieve the next edge, its "0 0-4", right ?
        iter0.next();
        // ouch! its not
        System.out.println(iter0);
        // iter0 and iter1 are independent right ?
        assertNotSame("NO THEY ARE NOT", iter0, iter1);
    }

The problem is that the EdgeExplorer basically returns itself (and NOT some kind of fresh iterator object) when we call setBaseNode. So after the second time calling setBaseNode the previous iterator (iter0) is more or less invalid (should not be used anymore, or actually it is just the same object as iter1). In fact iter0/1 is also the same object as the explorer.