Partitioner Improvements and Utils on top of #3603 (#3611)

* Implements Random Access Iterator Facade for EdgeIDIterator

* Makes StaticGraph Node and Edge requirements explicit

* Cleans up Bisection Graph, Node and Edge

* Cleans up GraphView

include/partition/bisection_graph.hpp

@@ -8,45 +8,54 @@
 #include "extractor/edge_based_edge.hpp"
 
 #include <cstddef>
+
+#include <algorithm>
+#include <iterator>
 #include <tuple>
+#include <utility>
 
 namespace osrm
 {
 namespace partition
 {
 
-// Required for usage in static graph
+// Graph node and its corresponding coordinate.
+// The coordinate will be used in the partitioning step.
 struct BisectionNode
 {
+    // StaticGraph Node requirement (see static graph traits): .first_edge
     std::size_t first_edge;
-    util::Coordinate cordinate;
+
+    util::Coordinate coordinate;
 };
 
-// The edge is used within a partition
+// Graph edge and data for Max-Flow Min-Cut augmentation.
 struct BisectionEdge
 {
+    // StaticGraph Edge requirement (see static graph traits): .target, .data
     NodeID target;
-    std::int32_t data; // will be one, but we have the space...
+
+    // TODO: add data for augmentation here. In case we want to keep it completely external, the
+    // static graph can be modified to no longer require a .data member by SFINAE-ing out features
+    // based on the available compile time traits.
+    std::int32_t data;
 };
 
-// workaround of how static graph assumes edges to be formatted :(
+// The graph layout we use as a basis for partitioning.
 using BisectionGraph = util::FlexibleStaticGraph<BisectionNode, BisectionEdge>;
 
-template <typename InputEdge> std::vector<InputEdge> groupBySource(std::vector<InputEdge> edges)
+template <typename RandomIt> void sortBySourceThenTarget(RandomIt first, RandomIt last)
 {
-    std::sort(edges.begin(), edges.end(), [](const auto &lhs, const auto &rhs) {
+    std::sort(first, last, [](const auto &lhs, const auto &rhs) {
         return std::tie(lhs.source, lhs.target) < std::tie(rhs.source, rhs.target);
     });
-
-    return edges;
 }
 
 template <typename InputEdge>
-std::vector<BisectionNode> computeNodes(const std::vector<util::Coordinate> coordinates,
+std::vector<BisectionNode> computeNodes(const std::vector<util::Coordinate> &coordinates,
                                         const std::vector<InputEdge> &edges)
 {
-    std::vector<BisectionNode> result;
-    result.reserve(coordinates.size() + 1);
+    std::vector<BisectionNode> result(coordinates.size() + 1 /*sentinel*/);
 
     // stateful transform, counting node ids and moving the edge itr forward
     const auto coordinate_to_bisection_node =
@@ -65,14 +74,13 @@ std::vector<BisectionNode> computeNodes(const std::vector<util::Coordinate> coor
         return {static_cast<std::size_t>(std::distance(edges.begin(), edges_of_node)), coordinate};
     };
 
-    std::transform(coordinates.begin(),
-                   coordinates.end(),
-                   std::back_inserter(result),
-                   coordinate_to_bisection_node);
+    std::transform(
+        begin(coordinates), end(coordinates), begin(result), coordinate_to_bisection_node);
 
-    // sentinel element
-    result.push_back(
-        {edges.size(), util::Coordinate(util::FloatLongitude{0.0}, util::FloatLatitude{0.0})});
+    auto null_island = util::Coordinate(util::FloatLongitude{0.0}, util::FloatLatitude{0.0});
+    auto sentinel = BisectionNode{edges.size(), std::move(null_island)};
+
+    result.back() = std::move(sentinel);
 
     return result;
 }
@@ -80,15 +88,11 @@ std::vector<BisectionNode> computeNodes(const std::vector<util::Coordinate> coor
 template <typename InputEdge>
 std::vector<BisectionEdge> adaptToBisectionEdge(std::vector<InputEdge> edges)
 {
-    std::vector<BisectionEdge> result;
+    std::vector<BisectionEdge> result(edges.size());
 
-    result.reserve(edges.size());
-    std::transform(edges.begin(),
-                   edges.end(),
-                   std::back_inserter(result),
-                   [](const auto &edge) -> BisectionEdge {
-                       return {edge.target, 1};
-                   });
+    std::transform(begin(edges), end(edges), begin(result), [](const auto &edge) {
+        return BisectionEdge{edge.target, 1};
+    });
 
     return result;
 }

include/partition/graph_view.hpp

@@ -7,11 +7,15 @@
 #include <boost/iterator/filter_iterator.hpp>
 #include <boost/iterator/iterator_facade.hpp>
 
+#include <cstddef>
+
 namespace osrm
 {
 namespace partition
 {
 
+// Predicate for EdgeIDs checking their partition ids for equality.
+// Used in filter iterator below to discard edges in different partitions.
 struct HasSamePartitionID
 {
     HasSamePartitionID(const RecursiveBisectionState::BisectionID bisection_id,
@@ -26,9 +30,10 @@ struct HasSamePartitionID
     const RecursiveBisectionState &recursive_bisection_state;
 };
 
-// a wrapper around the EdgeIDs returned by the static graph to make them iterable
-class EdgeIDIterator
-    : public boost::iterator_facade<EdgeIDIterator, EdgeID const, boost::random_access_traversal_tag>
+// Random Access Iterator on top of contiguous integral EdgeIDs
+class EdgeIDIterator : public boost::iterator_facade<EdgeIDIterator,
+                                                     EdgeID const,
+                                                     boost::random_access_traversal_tag>
 {
   public:
     EdgeIDIterator() : position(SPECIAL_EDGEID) {}
@@ -37,13 +42,24 @@ class EdgeIDIterator
   private:
     friend class boost::iterator_core_access;
 
-    void increment() { position++; }
-    bool equal(const EdgeIDIterator &other) const { return position == other.position; }
-    const EdgeID &dereference() const { return position; }
+    // Implements the facade's core operations required for random access iterators:
+    // http://www.boost.org/doc/libs/1_63_0/libs/iterator/doc/iterator_facade.html#core-operations
 
-    EdgeID position;
+    void increment() { ++position; }
+    void decrement() { --position; }
+    void advance(difference_type offset) { position += offset; }
+    bool equal(const EdgeIDIterator &other) const { return position == other.position; }
+    const reference dereference() const { return position; }
+    difference_type distance_to(const EdgeIDIterator &other) const
+    {
+        return static_cast<difference_type>(other.position - position);
+    }
+
+    value_type position;
 };
 
+// Non-owning immutable sub-graph view into a base graph.
+// The part of the graph to select is determined by the recursive bisection state.
 class GraphView
 {
   public:
@@ -54,6 +70,7 @@ class GraphView
               const RecursiveBisectionState::IDIterator begin,
               const RecursiveBisectionState::IDIterator end);
 
+    // Number of nodes _in this sub-graph_.
     std::size_t NumberOfNodes() const;
 
     RecursiveBisectionState::IDIterator Begin() const;

include/util/static_graph.hpp

@@ -4,6 +4,7 @@
 #include "util/integer_range.hpp"
 #include "util/percent.hpp"
 #include "util/shared_memory_vector_wrapper.hpp"
+#include "util/static_graph_traits.hpp"
 #include "util/typedefs.hpp"
 
 #include <boost/assert.hpp>
@@ -62,6 +63,11 @@ template <typename EdgeDataT> class SortableEdgeWithData
 
 template <typename NodeT, typename EdgeT, bool UseSharedMemory = false> class FlexibleStaticGraph
 {
+    static_assert(traits::HasFirstEdgeMember<NodeT>(),
+                  "Model for compatible Node type requires .first_edge member attribute");
+    static_assert(traits::HasDataAndTargetMember<EdgeT>(),
+                  "Model for compatible Edge type requires .data and .target member attribute");
+
   public:
     using NodeIterator = static_graph_details::NodeIterator;
     using EdgeIterator = static_graph_details::EdgeIterator;
@@ -210,8 +216,8 @@ template <typename NodeT, typename EdgeT, bool UseSharedMemory = false> class Fl
         return current_iterator;
     }
 
-    const NodeArrayEntry& GetNode(const NodeID nid) const { return node_array[nid]; }
-    const EdgeArrayEntry& GetEdge(const EdgeID eid) const { return edge_array[eid]; }
+    const NodeArrayEntry &GetNode(const NodeID nid) const { return node_array[nid]; }
+    const EdgeArrayEntry &GetEdge(const EdgeID eid) const { return edge_array[eid]; }
 
   private:
     NodeIterator number_of_nodes;

include/util/static_graph_traits.hpp

@@ -0,0 +1,56 @@
+#ifndef STATIC_GRAPH_TRAITS_HPP
+#define STATIC_GRAPH_TRAITS_HPP
+
+#include <type_traits>
+
+namespace osrm
+{
+namespace util
+{
+
+namespace traits
+{
+
+// Introspection for an arbitrary .data member attribute
+template <typename T, typename = void> struct HasDataMember : std::false_type
+{
+};
+
+template <typename T>
+struct HasDataMember<T, decltype((void)(sizeof(std::declval<T>().data) > 0))> : std::true_type
+{
+};
+
+// Introspection for an arbitrary .target member attribute
+template <typename T, typename = void> struct HasTargetMember : std::false_type
+{
+};
+
+template <typename T>
+struct HasTargetMember<T, decltype((void)(sizeof(std::declval<T>().target) > 0))> : std::true_type
+{
+};
+
+// Static Graph requires edges to have a .target and .data member attribute
+template <typename Edge>
+struct HasDataAndTargetMember
+    : std::integral_constant<bool, HasDataMember<Edge>() && HasTargetMember<Edge>()>
+{
+};
+
+// Static Graph requires nodes to have a .first_edge member attribute
+template <typename T, typename = void> struct HasFirstEdgeMember : std::false_type
+{
+};
+
+template <typename T>
+struct HasFirstEdgeMember<T, decltype((void)(sizeof(std::declval<T>().first_edge) > 0))>
+    : std::true_type
+{
+};
+
+} // ns traits
+} // ns util
+} // ns osrm
+
+#endif // STATIC_GRAPH_TRAITS_HPP

src/partition/graph_view.cpp

@@ -1,4 +1,6 @@
 #include "partition/graph_view.hpp"
+
+#include <iostream>
 #include <iterator>
 
 namespace osrm
@@ -25,12 +27,12 @@ GraphView::GraphView(const BisectionGraph &bisection_graph_,
                      const RecursiveBisectionState::IDIterator end_)
     : bisection_graph(bisection_graph_), bisection_state(bisection_state_), begin(begin_), end(end_)
 {
-    // print graph
+
     std::cout << "Graph\n";
-    for( auto itr = begin_; itr != end_; ++itr )
+    for (auto itr = begin; itr != end; ++itr)
     {
         std::cout << "Node: " << *itr << std::endl;
-        for( auto eitr = EdgeBegin(*itr); eitr != EdgeEnd(*itr); ++eitr )
+        for (auto eitr = EdgeBegin(*itr); eitr != EdgeEnd(*itr); ++eitr)
         {
             std::cout << "\t" << *eitr << " -> " << bisection_graph.GetTarget(*eitr) << std::endl;
         }
@@ -45,20 +47,23 @@ std::size_t GraphView::NumberOfNodes() const { return std::distance(begin, end);
 
 GraphView::EdgeIterator GraphView::EdgeBegin(const NodeID nid) const
 {
-    return boost::make_filter_iterator(
-        HasSamePartitionID(bisection_state.GetBisectionID(nid),
-                           bisection_graph,
-                           bisection_state),
-        EdgeIDIterator(bisection_graph.BeginEdges(nid)),
-        EdgeIDIterator(bisection_graph.EndEdges(nid)));
+    HasSamePartitionID predicate{
+        bisection_state.GetBisectionID(nid), bisection_graph, bisection_state};
+
+    EdgeIDIterator first{bisection_graph.BeginEdges(nid)};
+    EdgeIDIterator last{bisection_graph.EndEdges(nid)};
+
+    return boost::make_filter_iterator(predicate, first, last);
 }
 
 GraphView::EdgeIterator GraphView::EdgeEnd(const NodeID nid) const
 {
-    return boost::make_filter_iterator(
-        HasSamePartitionID(bisection_state.GetBisectionID(nid), bisection_graph, bisection_state),
-        EdgeIDIterator(bisection_graph.EndEdges(nid)),
-        EdgeIDIterator(bisection_graph.EndEdges(nid)));
+    HasSamePartitionID predicate{
+        bisection_state.GetBisectionID(nid), bisection_graph, bisection_state};
+
+    EdgeIDIterator last{bisection_graph.EndEdges(nid)};
+
+    return boost::make_filter_iterator(predicate, last, last);
 }
 
 } // namespace partition

src/partition/partitioner.cpp

@@ -6,6 +6,9 @@
 // TODO remove after testing
 #include "util/coordinate.hpp"
 
+#include <iterator>
+#include <vector>
+
 namespace osrm
 {
 namespace partition
@@ -53,7 +56,7 @@ int Partitioner::Run(const PartitionConfig &config)
     input_edges.push_back({7, 3});
     input_edges.push_back({7, 6});
 
-    input_edges = groupBySource(std::move(input_edges));
+    sortBySourceThenTarget(begin(input_edges), end(input_edges));
 
     std::vector<util::Coordinate> coordinates;