Fix for non-unique route summary #2996
Conversation
| @@ -18,9 +18,10 @@ using namespace valhalla::baldr; | |||
| using namespace valhalla::gurka; | |||
| using namespace valhalla::mjolnir; | |||
|
|
|||
| namespace { | |||
| TEST(TestRouteSummary, GetSummary) { | |||
There was a problem hiding this comment.
Sorry this diff looks weird, I basically moved the map building step into the test.
| @@ -79,15 +69,15 @@ TEST(TestRouteSummary, GetSummary) { | |||
| EXPECT_EQ(result1.trip().routes_size(), 1); | |||
| EXPECT_EQ(result1.trip().routes(0).legs_size(), 1); | |||
| gurka::assert::osrm::expect_steps(result1, {"RT 1", "RT 4", "RT 5"}); | |||
| gurka::assert::osrm::expect_summaries(result0, {"RT 1, RT 5"}); | |||
| gurka::assert::osrm::expect_summaries(result1, {"RT 1, RT 5"}); | |||
There was a problem hiding this comment.
There were a couple copy/paste errors in this preexisting test.
ktatterso
requested review from
purew,
merkispavel,
dgearhart,
kevinkreiser and
mandeepsandhu
| // The route's can be made unique by having the auto choose RT 3 and the ped choose RT 4 | ||
| // at the end of the route. | ||
|
|
||
| // Below we mash together the three above results into a single result. When we |
There was a problem hiding this comment.
looking below it seems like actually you wanted to have alternates that you needed to generate unique summaries for? you can get alternates by asking for it in the request "alternates":1. maybe the map above fails to find an alternate? anyway i think it would simplify and make the test a bit more understandable.
originally when i read the comment above you happened to use the word leg which made me think of multipoint route but that isnt what you meant, you really meant just a subsection (series of edges) of the route.
or have i confused what you meant?
There was a problem hiding this comment.
Hmm, that's a good suggestion about "alternates":1. However, wrestling gurka into the configuration I desire can sometimes take more time than expected. So if its alright I'd like to leave it as it is: not really testing whether we can generate alternates and produce unique summaries - but rather, just a test of unique summary generation.
And sorry, yes, I frequently catch myself typing comments with the term leg to just mean "some section of a route", forgetting it is an important reserved valhalla term. I'll keep working on that. 😅
There was a problem hiding this comment.
Oh i agree we arent testing whether or not we can generate alternates we are testing whether or not alternates have unique names. I just think that your test can be simplified to something like:
valhalla::Api result0 = gurka::do_action(valhalla::Options::route, map, {"A", "J"}, "auto", {{"alternates", "1"}});
EXPECT_EQ(result.trip().routes_size(), 2);
EXPECT_EQ(result.trip().routes(0).legs_size(), 1);
EXPECT_EQ(result.trip().routes(1).legs_size(), 1);
const std::string expected_route_summary0 = "RT 1, RT 2, RT 3";
const std::string expected_route_summary1 = "RT 1, RT 2, RT 4";
gurka::assert::osrm::expect_summaries(result, {expected_route_summary0, expected_route_summary1});I find that a heck of a lot easier to understand than the splicing etc
EDIT: Though i guess the map you drew above wont get alternates in the way you expect them in the test. i guess the point is that only a short section differs. i think you can get this by making a more trivial map that has two ways to go around the block. like just route from "E" to "J" and remove the pedestrian only attributes of "FKJ"
There was a problem hiding this comment.
I'll give this a quick shot... if it isn't quick I'd like to punt and move on with other things. Brb.
There was a problem hiding this comment.
I took a quick shot at modifying the test to actually compute alternates. Not sure why it didn't, I was still only getting one route. I was able to remove the pedestrian-only attributes, fwiw. I guess rather than spend more time simplifying this test (which sufficiently proves the fix), I'd like to move on with other things. thanks.
There was a problem hiding this comment.
no worries, thanks for taking a shot
| @@ -1616,7 +1616,7 @@ summarize_route_legs(const google::protobuf::RepeatedPtrField<DirectionsRoute>& | |||
|
|
|||
| // k is the number of named segments in the summary. It keeps going | |||
| // up by 1 until route_i's summary is different than the route_j's. | |||
| size_t k = 1; | |||
| size_t k = std::min(num_named_segs_needed, num_comparable); | |||
There was a problem hiding this comment.
you got to love these where the fix is one line but the test is like 100 lines 😄
Issue
Found an interesting case where two routes had the same summary, exposing a defect in the summary logic. The test case covers the exact reason for the issue and proves the resolution, but here's the gist of the issue:
Two routes end up getting to the same destination via different paths. The longest named segment in each path is different. The flaw in the logic was we stopped "unique-fying" the route summary once we saw the longest named segment was different. Later, since we desire route summaries to be comprised of at least two named segments (if possible) we append one more named segment to each - and as a result (in this case), the route summaries are no longer unique.
The fix is to set the desired minimum number of named segments per summary to two in the "unique-fying" step.
Tasklist