Bump version and adjust changelog

This ensures that small insignificat turns don't affect the overall
sharing metric too much.

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 5.17.3
+  - Changes from 5.17.2:
+    - Bugfixes:
+      - FIXED: Increased probability of returning alternatives and quality of the ones returned for MLD. [#5048](https://github.com/Project-OSRM/osrm-backend/issues/5048)
+
 # 5.17.2
   - Changes from 5.17.0:
     - Bugfixes:

package.json

@@ -1,6 +1,6 @@
 {
   "name": "osrm",
-  "version": "5.17.2",
+  "version": "5.17.3",
   "private": false,
   "description": "The Open Source Routing Machine is a high performance routing engine written in C++14 designed to run on OpenStreetMap data.",
   "dependencies": {

src/engine/routing_algorithms/alternative_path_mld.cpp

@@ -34,22 +34,27 @@ using Facade = DataFacade<Algorithm>;
 namespace
 {
 
-// Alternative paths candidate via nodes are taken from overlapping search spaces.
+struct Parameters
-// Overlapping by a third guarantees us taking candidate nodes "from the middle".
+{
-const constexpr auto kSearchSpaceOverlapFactor = 1.33;
+    // Alternative paths candidate via nodes are taken from overlapping search spaces.
-// Unpack n-times more candidate paths to run high-quality checks on.
+    // Overlapping by a third guarantees us taking candidate nodes "from the middle".
-// Unpacking paths yields higher chance to find good alternatives but is also expensive.
+    double kSearchSpaceOverlapFactor = 1.33;
-const constexpr auto kAlternativesToUnpackFactor = 2.0;
+    // Unpack n-times more candidate paths to run high-quality checks on.
-// Alternative paths length requirement (stretch).
+    // Unpacking paths yields higher chance to find good alternatives but is also expensive.
-// At most 25% longer then the shortest path.
+    unsigned kAlternativesToUnpackFactor = 2;
-const constexpr auto kAtMostLongerBy = 0.25;
+    // Alternative paths length requirement (stretch).
-// Alternative paths similarity requirement (sharing).
+    // At most 25% longer then the shortest path.
-// At least 15% different than the shortest path.
+    double kAtMostLongerBy = 0.25;
-const constexpr auto kAtLeastDifferentBy = 0.85;
+    // Alternative paths similarity requirement (sharing).
-// Alternative paths are still reasonable around the via node candidate (local optimality).
+    // At least 25% different than the shortest path.
-// At least optimal around 10% sub-paths around the via node candidate.
+    double kAtMostSameBy = 0.75;
-const /*constexpr*/ auto kAtLeastOptimalAroundViaBy = 0.10;
+    // Alternative paths are still reasonable around the via node candidate (local optimality).
-// gcc 7.1 ICE ^
+    // At least optimal around 10% sub-paths around the via node candidate.
+    double kAtLeastOptimalAroundViaBy = 0.1;
+    // Alternative paths similarity requirement (sharing) based on calles.
+    // At least 15% different than the shortest path.
+    double kCellsAtMostSameBy = 0.95;
+};
 
 // Represents a via middle node where forward (from s) and backward (from t)
 // search spaces overlap and the weight a path (made up of s,via and via,t) has.
@@ -71,11 +76,104 @@ struct WeightedViaNodePackedPath
 // its total weight and the via node used to construct the path.
 struct WeightedViaNodeUnpackedPath
 {
+    double sharing;
     WeightedViaNode via;
     UnpackedNodes nodes;
     UnpackedEdges edges;
 };
 
+// Scale the maximum allowed weight increase based on its magnitude:
+//  - Shortest path 10 minutes, alternative 13 minutes => Factor of 0.30 ok
+//  - Shortest path 10 hours, alternative 13 hours     => Factor of 0.30 unreasonable
+double getLongerByFactorBasedOnDuration(const EdgeWeight duration)
+{
+    BOOST_ASSERT(duration != INVALID_EDGE_WEIGHT);
+
+    // We only have generic weights here and no durations without unpacking.
+    // We also have restricted way penalties which are huge and will screw scaling here.
+    //
+    // Users can pass us generic weights not based on durations; we can't do anything about
+    // it here other than either generating too many or no alternatives in these cases.
+    //
+    // We scale the weights with a step function based on some rough guestimates, so that
+    // they match tens of minutes, in the low hours, tens of hours, etc.
+
+    // Computed using a least-squares curve fitiing on the following values:
+    //
+    // def func(xs, a, b, c, d):
+    //       return a + b/(xs-d) + c/(xs-d)**3
+    //
+    // xs = np.array([5 * 60, 10 * 60, 30 * 60, 60 * 60, 3 * 60 * 60, 10 * 60 * 60])
+    // ys = np.array([1.0,    0.75,    0.5,     0.4,     0.3,          0.2])
+    //
+    // xs_interp = np.arange(5*60, 10*60*60, 5*60)
+    // ys_interp = np.interp(xs_interp, xs, ys)
+    //
+    // params, _ = scipy.optimize.curve_fit(func, xs_interp, ys_interp)
+    //
+    // The hyperbolic shape was chosen because it interpolated well between
+    // the given datapoints and drops off for large durations.
+    const constexpr auto a = 1.91578463e-01;
+    const constexpr auto b = 1.35118442e+03;
+    const constexpr auto c = 2.45437877e+09;
+    const constexpr auto d = -2.07944571e+03;
+
+    if (duration < EdgeWeight(5 * 60))
+    {
+        return 1.0;
+    }
+    else if (duration > EdgeWeight(10 * 60 * 60))
+    {
+        return 0.20;
+    }
+
+    // Bigger than 10 minutes but smaller than 10 hours
+    BOOST_ASSERT(duration >= 5 * 60 && duration <= 10 * 60 * 60);
+
+    return a + b / (duration - d) + c / std::pow(duration - d, 3);
+}
+
+Parameters parametersFromRequest(const PhantomNodes &phantom_node_pair)
+{
+    Parameters parameters;
+
+    const auto distance = util::coordinate_calculation::haversineDistance(
+        phantom_node_pair.source_phantom.location, phantom_node_pair.target_phantom.location);
+
+    // 10km
+    if (distance < 10000.)
+    {
+        parameters.kAlternativesToUnpackFactor = 10.0;
+        parameters.kCellsAtMostSameBy = 1.0;
+        parameters.kAtLeastOptimalAroundViaBy = 0.2;
+        parameters.kAtMostSameBy = 0.50;
+    }
+    // 20km
+    else if (distance < 20000.)
+    {
+        parameters.kAlternativesToUnpackFactor = 8.0;
+        parameters.kCellsAtMostSameBy = 1.0;
+        parameters.kAtLeastOptimalAroundViaBy = 0.2;
+        parameters.kAtMostSameBy = 0.60;
+    }
+    // 50km
+    else if (distance < 50000.)
+    {
+        parameters.kAlternativesToUnpackFactor = 6.0;
+        parameters.kCellsAtMostSameBy = 0.95;
+        parameters.kAtMostSameBy = 0.65;
+    }
+    // 100km
+    else if (distance < 100000.)
+    {
+        parameters.kAlternativesToUnpackFactor = 4.0;
+        parameters.kCellsAtMostSameBy = 0.95;
+        parameters.kAtMostSameBy = 0.70;
+    }
+
+    return parameters;
+}
+
 // Filters candidates which are on not unique.
 // Returns an iterator to the uniquified range's new end.
 // Note: mutates the range in-place invalidating iterators.
@@ -110,49 +208,13 @@ RandIt filterViaCandidatesByRoadImportance(RandIt first, RandIt last, const Faca
     return last;
 }
 
-// Scale the maximum allowed weight increase based on its magnitude:
-//  - Shortest path 10 minutes, alternative 13 minutes => Factor of 0.30 ok
-//  - Shortest path 10 hours, alternative 13 hours     => Factor of 0.30 unreasonable
-double scaledAtMostLongerByFactorBasedOnDuration(EdgeWeight duration)
-{
-    BOOST_ASSERT(duration != INVALID_EDGE_WEIGHT);
-
-    // We only have generic weights here and no durations without unpacking.
-    // We also have restricted way penalties which are huge and will screw scaling here.
-    //
-    // Users can pass us generic weights not based on durations; we can't do anything about
-    // it here other than either generating too many or no alternatives in these cases.
-    //
-    // We scale the weights with a step function based on some rough guestimates, so that
-    // they match tens of minutes, in the low hours, tens of hours, etc.
-
-    // Todo: instead of a piecewise constant function should this be a continuous function?
-    // At the moment there are "hard" jump edge cases when crossing the thresholds.
-
-    auto scaledAtMostLongerBy = kAtMostLongerBy;
-
-    const constexpr auto minutes = 60.;
-    const constexpr auto hours = 60. * minutes;
-
-    if (duration < EdgeWeight(10 * minutes))
-        scaledAtMostLongerBy *= 1.20;
-    else if (duration < EdgeWeight(30 * minutes))
-        scaledAtMostLongerBy *= 1.00;
-    else if (duration < EdgeWeight(1 * hours))
-        scaledAtMostLongerBy *= 0.90;
-    else if (duration < EdgeWeight(3 * hours))
-        scaledAtMostLongerBy *= 0.70;
-    else if (duration > EdgeWeight(10 * hours))
-        scaledAtMostLongerBy *= 0.50;
-
-    return scaledAtMostLongerBy;
-}
-
 // Filters candidates with much higher weight than the primary route. Mutates range in-place.
 // Returns an iterator to the filtered range's new end.
 template <typename RandIt>
-RandIt
+RandIt filterViaCandidatesByStretch(RandIt first,
-filterViaCandidatesByStretch(RandIt first, RandIt last, EdgeWeight weight, double weight_multiplier)
+                                    RandIt last,
+                                    const EdgeWeight weight,
+                                    const Parameters &parameters)
 {
     util::static_assert_iter_category<RandIt, std::random_access_iterator_tag>();
     util::static_assert_iter_value<RandIt, WeightedViaNode>();
@@ -160,9 +222,7 @@ filterViaCandidatesByStretch(RandIt first, RandIt last, EdgeWeight weight, doubl
     // Assumes weight roughly corresponds to duration-ish. If this is not the case e.g.
     // because users are setting weight to be distance in the profiles, then we might
     // either generate more candidates than we have to or not enough. But is okay.
-    const auto scaled_at_most_longer_by =
+    const auto stretch_weight_limit = (1. + parameters.kAtMostLongerBy) * weight;
-        scaledAtMostLongerByFactorBasedOnDuration(weight / weight_multiplier);
-    const auto stretch_weight_limit = (1. + scaled_at_most_longer_by) * weight;
 
     const auto over_weight_limit = [=](const auto via) {
         return via.weight > stretch_weight_limit;
@@ -198,8 +258,15 @@ filterViaCandidatesByViaNotOnPath(const WeightedViaNodePackedPath &path, RandIt
 // Filters packed paths with similar cells between each other. Mutates range in-place.
 // Returns an iterator to the filtered range's new end.
 template <typename RandIt>
-RandIt filterPackedPathsByCellSharing(RandIt first, RandIt last, const Partition &partition)
+RandIt filterPackedPathsByCellSharing(RandIt first,
+                                      RandIt last,
+                                      const Partition &partition,
+                                      const Parameters &parameters)
 {
+    // In this case we don't need to calculate sharing, because it would not filter anything
+    if (parameters.kCellsAtMostSameBy >= 1.0)
+        return last;
+
     util::static_assert_iter_category<RandIt, std::random_access_iterator_tag>();
     util::static_assert_iter_value<RandIt, WeightedViaNodePackedPath>();
 
@@ -226,14 +293,13 @@ RandIt filterPackedPathsByCellSharing(RandIt first, RandIt last, const Partition
         return last;
 
     std::unordered_set<CellID> cells;
-    cells.reserve(size * (shortest_path.path.size() + 1) * (1. + kAtMostLongerBy));
+    cells.reserve(size * (shortest_path.path.size() + 1) * (1 + parameters.kAtMostLongerBy));
 
     cells.insert(get_cell(std::get<0>(shortest_path.path.front())));
     for (const auto &edge : shortest_path.path)
         cells.insert(get_cell(std::get<1>(edge)));
 
     const auto over_sharing_limit = [&](const auto &packed) {
-
         if (packed.path.empty())
         { // don't remove routes with single-node (empty) path
             return false;
@@ -253,7 +319,7 @@ RandIt filterPackedPathsByCellSharing(RandIt first, RandIt last, const Partition
 
         const auto sharing = 1. - difference;
 
-        if (sharing > kAtLeastDifferentBy)
+        if (sharing > parameters.kCellsAtMostSameBy)
         {
             return true;
         }
@@ -277,7 +343,8 @@ RandIt filterPackedPathsByLocalOptimality(const WeightedViaNodePackedPath &path,
                                           const Heap &forward_heap,
                                           const Heap &reverse_heap,
                                           RandIt first,
-                                          RandIt last)
+                                          RandIt last,
+                                          const Parameters &parameters)
 {
     util::static_assert_iter_category<RandIt, std::random_access_iterator_tag>();
     util::static_assert_iter_value<RandIt, WeightedViaNodePackedPath>();
@@ -376,7 +443,7 @@ RandIt filterPackedPathsByLocalOptimality(const WeightedViaNodePackedPath &path,
         const auto detour_length = forward_heap.GetKey(via) - forward_heap.GetKey(a) +
                                    reverse_heap.GetKey(via) - reverse_heap.GetKey(b);
 
-        return plateaux_length < kAtLeastOptimalAroundViaBy * detour_length;
+        return plateaux_length < parameters.kAtLeastOptimalAroundViaBy * detour_length;
     };
 
     return std::remove_if(first, last, is_not_locally_optimal);
@@ -384,7 +451,11 @@ RandIt filterPackedPathsByLocalOptimality(const WeightedViaNodePackedPath &path,
 
 // Filters unpacked paths compared to all other paths. Mutates range in-place.
 // Returns an iterator to the filtered range's new end.
-template <typename RandIt> RandIt filterUnpackedPathsBySharing(RandIt first, RandIt last)
+template <typename RandIt>
+RandIt filterUnpackedPathsBySharing(RandIt first,
+                                    RandIt last,
+                                    const Facade &facade,
+                                    const Parameters &parameters)
 {
     util::static_assert_iter_category<RandIt, std::random_access_iterator_tag>();
     util::static_assert_iter_value<RandIt, WeightedViaNodeUnpackedPath>();
@@ -400,27 +471,35 @@ template <typename RandIt> RandIt filterUnpackedPathsBySharing(RandIt first, Ran
         return last;
 
     std::unordered_set<EdgeID> edges;
-    edges.reserve(size * shortest_path.edges.size() * (1. + kAtMostLongerBy));
+    edges.reserve(size * shortest_path.edges.size() * (1.25));
 
-    edges.insert(begin(shortest_path.edges), begin(shortest_path.edges));
+    edges.insert(begin(shortest_path.edges), end(shortest_path.edges));
-
-    const auto over_sharing_limit = [&](const auto &unpacked) {
 
+    const auto over_sharing_limit = [&](auto &unpacked) {
         if (unpacked.edges.empty())
         { // don't remove routes with single-node (empty) path
             return false;
         }
 
-        const auto not_seen = [&](const EdgeID edge) { return edges.count(edge) < 1; };
+        EdgeWeight total_duration = 0;
-        const auto different = std::count_if(begin(unpacked.edges), end(unpacked.edges), not_seen);
+        const auto add_if_seen = [&](const EdgeWeight duration, const EdgeID edge) {
+            auto edge_duration = facade.GetEdgeData(edge).duration;
+            total_duration += edge_duration;
+            if (edges.count(edge) > 0)
+            {
+                return duration + edge_duration;
+            }
+            return duration;
+        };
 
-        const auto difference = different / static_cast<double>(unpacked.edges.size());
+        const auto shared_weight =
-        BOOST_ASSERT(difference >= 0.);
+            std::accumulate(begin(unpacked.edges), end(unpacked.edges), EdgeWeight{0}, add_if_seen);
-        BOOST_ASSERT(difference <= 1.);
 
-        const auto sharing = 1. - difference;
+        unpacked.sharing = shared_weight / static_cast<double>(total_duration);
+        BOOST_ASSERT(unpacked.sharing >= 0.);
+        BOOST_ASSERT(unpacked.sharing <= 1.);
 
-        if (sharing > kAtLeastDifferentBy)
+        if (unpacked.sharing > parameters.kAtMostSameBy)
         {
             return true;
         }
@@ -431,14 +510,19 @@ template <typename RandIt> RandIt filterUnpackedPathsBySharing(RandIt first, Ran
         }
     };
 
-    return std::remove_if(first + 1, last, over_sharing_limit);
+    auto end = std::remove_if(first + 1, last, over_sharing_limit);
+    std::sort(
+        first + 1, end, [](const auto &lhs, const auto &rhs) { return lhs.sharing < rhs.sharing; });
+    return end;
 }
 
 // Filters annotated routes by stretch based on duration. Mutates range in-place.
 // Returns an iterator to the filtered range's new end.
 template <typename RandIt>
-RandIt
+RandIt filterAnnotatedRoutesByStretch(RandIt first,
-filterAnnotatedRoutesByStretch(RandIt first, RandIt last, const InternalRouteResult &shortest_route)
+                                      RandIt last,
+                                      const InternalRouteResult &shortest_route,
+                                      const Parameters &parameters)
 {
     util::static_assert_iter_category<RandIt, std::random_access_iterator_tag>();
     util::static_assert_iter_value<RandIt, InternalRouteResult>();
@@ -446,9 +530,7 @@ filterAnnotatedRoutesByStretch(RandIt first, RandIt last, const InternalRouteRes
     BOOST_ASSERT(shortest_route.is_valid());
 
     const auto shortest_route_duration = shortest_route.duration();
-    const auto scaled_at_most_longer_by =
+    const auto stretch_duration_limit = (1. + parameters.kAtMostLongerBy) * shortest_route_duration;
-        scaledAtMostLongerByFactorBasedOnDuration(shortest_route_duration);
-    const auto stretch_duration_limit = (1. + scaled_at_most_longer_by) * shortest_route_duration;
 
     const auto over_duration_limit = [=](const auto &route) {
         return route.duration() > stretch_duration_limit;
@@ -559,6 +641,7 @@ void unpackPackedPaths(InputIt first,
         }
 
         WeightedViaNodeUnpackedPath unpacked_path{
+            0.0,
             WeightedViaNode{packed_path_via, packed_path_weight},
             std::move(unpacked_nodes),
             std::move(unpacked_edges)};
@@ -573,7 +656,8 @@ void unpackPackedPaths(InputIt first,
 inline std::vector<WeightedViaNode>
 makeCandidateVias(SearchEngineData<Algorithm> &search_engine_data,
                   const Facade &facade,
-                  const PhantomNodes &phantom_node_pair)
+                  const PhantomNodes &phantom_node_pair,
+                  const Parameters &parameters)
 {
     Heap &forward_heap = *search_engine_data.forward_heap_1;
     Heap &reverse_heap = *search_engine_data.reverse_heap_1;
@@ -605,7 +689,7 @@ makeCandidateVias(SearchEngineData<Algorithm> &search_engine_data,
     while (forward_heap.Size() + reverse_heap.Size() > 0)
     {
         if (shortest_path_weight != INVALID_EDGE_WEIGHT)
-            overlap_weight = shortest_path_weight * kSearchSpaceOverlapFactor;
+            overlap_weight = shortest_path_weight * parameters.kSearchSpaceOverlapFactor;
 
         // Termination criteria - when we have a shortest path this will guarantee for our overlap.
         const bool keep_going = forward_heap_min + reverse_heap_min < overlap_weight;
@@ -670,7 +754,7 @@ makeCandidateVias(SearchEngineData<Algorithm> &search_engine_data,
     return candidate_vias;
 }
 
-} // anon. ns
+} // namespace
 
 // Alternative Routes for MLD.
 //
@@ -691,9 +775,11 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
                                                const PhantomNodes &phantom_node_pair,
                                                unsigned number_of_alternatives)
 {
+    Parameters parameters = parametersFromRequest(phantom_node_pair);
+
     const auto max_number_of_alternatives = number_of_alternatives;
     const auto max_number_of_alternatives_to_unpack =
-        kAlternativesToUnpackFactor * max_number_of_alternatives;
+        parameters.kAlternativesToUnpackFactor * max_number_of_alternatives;
     BOOST_ASSERT(max_number_of_alternatives > 0);
     BOOST_ASSERT(max_number_of_alternatives_to_unpack >= max_number_of_alternatives);
 
@@ -707,7 +793,8 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
     Heap &reverse_heap = *search_engine_data.reverse_heap_1;
 
     // Do forward and backward search, save search space overlap as via candidates.
-    auto candidate_vias = makeCandidateVias(search_engine_data, facade, phantom_node_pair);
+    auto candidate_vias =
+        makeCandidateVias(search_engine_data, facade, phantom_node_pair, parameters);
 
     const auto by_weight = [](const auto &lhs, const auto &rhs) { return lhs.weight < rhs.weight; };
     auto shortest_path_via_it =
@@ -730,6 +817,9 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
     NodeID shortest_path_via = shortest_path_via_it->node;
     EdgeWeight shortest_path_weight = shortest_path_via_it->weight;
 
+    const double duration_estimation = shortest_path_weight / facade.GetWeightMultiplier();
+    parameters.kAtMostLongerBy = getLongerByFactorBasedOnDuration(duration_estimation);
+
     // Filters via candidate nodes with heuristics
 
     // Note: filter pipeline below only makes range smaller; no need to erase items
@@ -738,8 +828,7 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
 
     it = filterViaCandidatesByUniqueNodeIds(begin(candidate_vias), it);
     it = filterViaCandidatesByRoadImportance(begin(candidate_vias), it, facade);
-    it = filterViaCandidatesByStretch(
+    it = filterViaCandidatesByStretch(begin(candidate_vias), it, shortest_path_weight, parameters);
-        begin(candidate_vias), it, shortest_path_weight, facade.GetWeightMultiplier());
 
     // Pre-rank by weight; sharing filtering below then discards by similarity.
     std::sort(begin(candidate_vias), it, [](const auto lhs, const auto rhs) {
@@ -783,10 +872,10 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
                                                                 forward_heap, // paths for s, via
                                                                 reverse_heap, // paths for via, t
                                                                 begin(weighted_packed_paths) + 1,
-                                                                alternative_paths_last);
+                                                                alternative_paths_last,
-
+                                                                parameters);
     alternative_paths_last = filterPackedPathsByCellSharing(
-        begin(weighted_packed_paths), alternative_paths_last, partition);
+        begin(weighted_packed_paths), alternative_paths_last, partition, parameters);
 
     BOOST_ASSERT(weighted_packed_paths.size() >= 1);
 
@@ -816,7 +905,8 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
 
     auto unpacked_paths_last = end(unpacked_paths);
 
-    unpacked_paths_last = filterUnpackedPathsBySharing(begin(unpacked_paths), end(unpacked_paths));
+    unpacked_paths_last = filterUnpackedPathsBySharing(
+        begin(unpacked_paths), end(unpacked_paths), facade, parameters);
 
     const auto unpacked_paths_first = begin(unpacked_paths);
     const auto number_of_unpacked_paths =
@@ -850,7 +940,9 @@ InternalManyRoutesResult alternativePathSearch(SearchEngineData<Algorithm> &sear
 
     if (routes.size() > 1)
     {
-        routes_last = filterAnnotatedRoutesByStretch(routes_first + 1, routes_last, *routes_first);
+        parameters.kAtMostLongerBy = getLongerByFactorBasedOnDuration(routes_first->duration());
+        routes_last = filterAnnotatedRoutesByStretch(
+            routes_first + 1, routes_last, *routes_first, parameters);
         routes.erase(routes_last, end(routes));
     }