Chunnel Routing #3038
Chunnel Routing #3038
Conversation
|
I think not reclassifying around short ferries was done to avoid some river ferries that had plenty of alternate paths. I think this was before any ferry penalties might have been in place. |
|
@kevinkreiser checking on a status for you. |
|
@kevinkreiser just curious about weights. If I use intermediate point to force ferry or chunnel I get chunnel route to be like 25% more expensive than the ferry one. Is it different for you? I'm checking regarding ferry connections reclassification now, but even if it's the case, difference in weight will still prevent us from getting adequate routes generally |
|
@kevinkreiser So I think there IS a problem with ferry connections. GB side pictures: current connection(s) with chunnel edges omitted, entering and exiting the chunnel. The ‘exiting’, is the only one that follows level 0 (red) edges. ‘Entering’ is a trash (and causes “search exhausted” stuff). To me it looks like the connection is built correctly only in one direction and not in reverse. Something similar happens on the France end as well. Edges by level: |
|
Also the France side: |
|
@SvetlanaBayarovich yep that is what we saw. we are pretty sure about the issue and actually @rzyc made a unit test to show why this is happening and the fix. i'll let him explain it, he should be pushing to this branch shortly |
|
@SvetlanaBayarovich The issue was in the In this PR, I added the "just one edge" check by looping over all other connected edges (at both source and destination nodes) and seeing if any of them were also ferry edges. The function returns |
| // If we notice that either the end node or source node is a connection to another ferry edge, | ||
| // be cautious and assume this isn't a short edge. | ||
| for (const auto& edge2 : bundle.node_edges) { | ||
| if (edge2.first.llindex_ != edge.first.llindex_ && edge2.first.attributes.driveable_ferry) { | ||
| return false; | ||
| } | ||
| } | ||
| for (const auto& edge2 : bundle2.node_edges) { | ||
| if (edge2.first.llindex_ != edge.first.llindex_ && edge2.first.attributes.driveable_ferry) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The "just one edge" check. Looping over all other connected edges at both source and destination nodes. Sees if any of them were also ferry edges, with a check on the latlong index to make sure we're not looking at the original ferry edge. Return false immediately if we find one, since we know that means this ferry consists of more than one edge.
| ReclassifyFerryConnectionEdge(std::map<std::string, std::string> const& way_description) { | ||
| constexpr double gridsize_metres = 1000; | ||
|
|
||
| const std::string ascii_map = R"( | ||
| A--B--b--C----D--E | ||
| F--G--g--H----I--J |
There was a problem hiding this comment.
Adding a parallel test case ensuring that the behavior for rail ferries is the same as for ferries. The only difference in these two test cases is that HI is a rail ferry and CD is a ferry.
| const std::string ascii_map = R"( | ||
| A--B--C--F--D--E--G | ||
| | | ||
| N | ||
| )"; |
There was a problem hiding this comment.
Adding a test case here to cover the issue discovered. Both CF and FD are ferry edges, but they are both < 2km. This means that ShortFerry should return false since the ferry consists of > 1 edge. Thus, we should be reclassifying BC and DE and we check that in this unit test.
Also made sure that this unit test was failing if we temporarily removed this PR's changes to ShortFerry.
There was a problem hiding this comment.
yeah the intuition here is we shouldnt assume the ferry is short just because it has 1 short edge in it. seems like somewhere along the chunnel another drivable edge intersects it and so makes the first edge short but then of course the whole rest of the chunnel is many km long. basically shortFerry was tricked. it still might be better to completely disable shortFerry, you can think of malicious cases where a short single edge ferry is the only way to get from one place to another but it likely doesnt happen in the wild that often (or maybe ever)
There was a problem hiding this comment.
You may never know when it appears)
| } | ||
| } | ||
|
|
||
| // If the end node has a non-ferry edge check the length of the edge | ||
| if (bundle2.node.non_ferry_edge_) { |
There was a problem hiding this comment.
The code previously had this if statement here as the "just one edge" check, but it wasn't sufficient. If we consider this case (used in the unit test described below):
A--B--C--F--D--E--G
|
N
where CF and FD are ferry edges and FN is not a ferry edge. Suppose that ShortFerry is analyzing CF and bundle2 is created at F. Then bundle2.node.non_ferry_edge_ will be true since FN exists.
There was a problem hiding this comment.
Haha, guess it was working in most cases because not typical for a 'traditional' ferry to have a non-ferry edge somewhere in the middle
Disregard what i just said 😅
|
@rzyc Wow that was a tricky one! And such an interesting intersection of different circumstances to make it happen in the wild. |
|
We're going to do a round of RAD before we merge this to make sure no other unexpected changes occur. |
|
Did a regression test with @kevinkreiser to check differences in routing before and after this PR. We did not notice any particularly drastic routing differences; most differences seem to be one-offs with just a couple slightly differing instructions ending up with similar cost values. One interesting route change involving a ferry was this:
Both routes have similar costs, but we noticed that after this fix with ferry reclassification, it elected to get on the ferry earlier in the route. Since there aren't any drastic differences, we are looking to merge this PR on Monday. |
|
@mandeep, looks like macos builds "just work" again, im going to maek them required again |
@kevinkreiser yeah. I got an email notification from circle saying they are updating all macos executors to use newer homebrew version. So looks like that did the trick. Glad its working without needing any change from us. |
Every now and then the Chunnel stops routing properly...
Recently (2 months ago) I made a planet build and the chunnel isn't the suggested route when going from the UK to France even if you put the origin and destination right at the chunnel entry and exit. Instead the route goes way south first and crosses on a ferry to France and then drives up north again to get back to the chunnel exit.
@rzyc and I debugged this for a while and found out a couple of things. First we found that if we took an extract of the planet that has only the parts of the uk and france with the chunnel connections in them the route still fails first and then succeeds on the second pass in routing_action.cc
then we started commenting out the 3 different relaxation types in the second pass. first the not_through and the destination_only stuff. the route still succeeded on the second pass. then we commented out the hierarchy relaxation and it failed the route.
this made me think that it had to do with reclassification. but ive added a unit test here that proves the reclassification does work for the chunnels tagging (route = shuttle_train).
after looking at the ferry reclassification for a little bit it seems that indeed the chunnels surrounding edges are not getting reclassified. what happen is that this function https://github.com/valhalla/valhalla/blob/master/src/mjolnir/ferry_connections.cc#L209 seems to think the chunnel is too short to be worth reclassifying for. if i disable this, routing works as expected (although the reclassified path it takes is kind of crappy (has a uturn)).
@dnesbitt61 what was the purpose of not reclassifying near short ferries? were we assuming there must be alternate ways around without taking the ferries themselves? it looks to me like maybe the chunnel is connected to a bunch of little edges in the queueing area on the british side and that might be triggering the shortferry logic to stop us from connecting up the chunnel.
@gknisely have you been suffering with the chunnel at all lately?