[WIP] Graph Concepts and Containers #634

fuchsto · 2019-03-09T17:55:50Z

As the STL has no equivalent, I defined the DASH graph concepts following the Boost Graph Library (BGL).
In his Master thesis, @stiefn refined and extended concepts for several peculiarities and requirements of PGAS and DASH.
This merge contains his reference implementation. Merging current development was surprisingly non-conflicting.
The implementation proved stable and effizient in several productive scenarios like BFS and Connected Components.

Missing: more unit tests, review, some suggestions for improvement in comments here and there.

…cept

…graph

codecov · 2019-03-12T20:02:58Z

Codecov Report

Merging #634 into development will increase coverage by 0.65%.
The diff coverage is 95.24%.

@@               Coverage Diff               @@
##           development     #634      +/-   ##
===============================================
+ Coverage        85.08%   85.74%   +0.65%     
===============================================
  Files              336      355      +19     
  Lines            24954    26570    +1616     
  Branches         11338    12040     +702     
===============================================
+ Hits             21232    22782    +1550     
- Misses            3692     3771      +79     
+ Partials            30       17      -13

Impacted Files	Coverage Δ
dash/test/algorithm/ConnectedComponentsTest.h	`100% <100%> (ø)`
dash/include/dash/memory/GlobHeapLocalPtr.h	`95.65% <100%> (+0.06%)`	⬆️
dash/include/dash/memory/GlobHeapCombinedMem.h	`100% <100%> (ø)`
dash/include/dash/graph/VertexIterator.h	`100% <100%> (ø)`
dash/test/algorithm/MinimumSpanningTreeTest.h	`100% <100%> (ø)`
dash/include/dash/memory/GlobHeapCombinedPtr.h	`100% <100%> (ø)`
dash/include/dash/graph/OutEdgeIterator.h	`100% <100%> (ø)`
dash/include/dash/graph/EdgeIterator.h	`100% <100%> (ø)`
dash/include/dash/graph/InEdgeIterator.h	`100% <100%> (ø)`
dash/test/container/GraphTest.h	`100% <100%> (ø)`
... and 33 more

fuchsto · 2019-03-12T21:01:01Z

@stiefn @fuerlinger @rkowalewski @devreal @pascalj

Graph containers and algorithms are fully merged now, including tests and examples.
Algorithms are Minimum Spanning Tree and Connected Components.

Relevant implementation is in:

dash/graph
dash/algorithms/graph

Integration tests passed, but could use some more unit tests.

Still, I'm voting for merging it into development, that's what the branch is about.

devreal · 2019-03-12T23:13:02Z

I will give the review a try tomorrow on the train.

bertwesarg · 2019-03-13T12:32:57Z

dash/include/dash/Graph.h

+  typedef Vertex<self_t>                              vertex_type;
+  typedef Edge<self_t>                                edge_type;
+
+  typedef std::vector<vertex_type> vertex_container_type;


Why is _VertexContainer not used? Same for edge_container_type.

bertwesarg · 2019-03-13T12:33:07Z

dash/include/dash/Graph.h

+  typename EdgeProperties   = EmptyProperties,  // user-defined struct
+  typename VertexSizeType   = int,
+  typename EdgeSizeType     = int,
+//template<class, class...> typename EdgeContainer   = std::vector,


remove dead code

bertwesarg · 2019-03-13T12:33:45Z

dash/include/dash/Graph.h

+  typename EdgeSizeType     = int,
+//template<class, class...> typename EdgeContainer   = std::vector,
+//template<class, class...> typename VertexContainer = std::vector
+  template<class, class...> class _EdgeContainer   = std::vector,


order is reversed, above its always vertex, than edge, here it is edge, than vertex

bertwesarg · 2019-03-13T12:36:10Z

dash/include/dash/Graph.h

+  GraphDirection Direction  = DirectedGraph,
+  typename VertexProperties = EmptyProperties,  // user-defined struct
+  typename EdgeProperties   = EmptyProperties,  // user-defined struct
+  typename VertexSizeType   = int,


enforce an unsigned type

bertwesarg · 2019-03-13T12:41:26Z

dash/include/dash/Graph.h

+  /**
+   * Removes a given vertex.
+   */
+  void remove_vertex(const local_vertex_iterator & v) {


unimplemented

bertwesarg · 2019-03-13T12:41:49Z

dash/include/dash/Graph.h

+  /**
+   * Removes the edges between two given vertices.
+   */
+  void remove_edge(const local_vertex_iterator & v1, 


unimplemented

bertwesarg · 2019-03-13T12:41:58Z

dash/include/dash/Graph.h

+  /**
+   * Removes the edges between two given vertices.
+   */
+  void remove_edge(const global_vertex_iterator & v1, 


unimplemented

bertwesarg · 2019-03-13T12:42:06Z

dash/include/dash/Graph.h

+  /**
+   * Removes a given edge.
+   */
+  void remove_edge(const local_out_edge_iterator & e) {


unimplemented

bertwesarg · 2019-03-13T12:42:14Z

dash/include/dash/Graph.h

+  /**
+   * Removes a given edge.
+   */
+  void remove_edge(const global_out_edge_iterator & e) {


unimplemented

bertwesarg · 2019-03-13T12:44:27Z

dash/include/dash/Graph.h

+  }
+  */
+
+  // TODO: Generalize following methods for other edge attributes than { int }


does this limit the usability of the whole Graph, or are these just convenience functions for the user? I.e, they are not used by the implementation itself?

devreal

I cannot claim to have read every line of code but overall I think the PR is in a good shape already. Documentation is rather scarce imo and can be improved. I see some code duplication that maybe can be avoided. Some minor and not-so-minor comments inline. Also, please add an entry to the CHANGELOG :)

devreal · 2019-03-13T15:05:44Z

dart-impl/mpi/src/dart_communication.c

+                   "dart_adapt_teamlist_convert failed", teamid);
+    return DART_ERR_INVAL;
+  }
+  if (sendbuf == recvbuf || NULL == sendbuf) {


Please be consistent with the change in dart_allgatherv above.

devreal · 2019-03-13T15:05:48Z

dart-impl/mpi/src/dart_communication.c

@@ -2168,6 +2168,150 @@ dart_ret_t dart_allgatherv(
  return DART_OK;
 }

+#if 0


Why is it disabled?

devreal · 2019-03-13T15:08:50Z

dart-impl/mpi/src/dart_communication.c

+  int *isenddispls  = malloc(sizeof(int) * comm_size);
+  int *inrecvcounts = malloc(sizeof(int) * comm_size);
+  int *irecvdispls  = malloc(sizeof(int) * comm_size);
+  for (int i = 0; i < comm_size; i++) {


This conversion (size_t -> int) pains me. It would be good to extract it and use a common implementation everywhere.

devreal · 2019-03-13T15:09:37Z

dash/examples/ex.02.graph-deref/main.cpp

+
+typedef dash::Graph<dash::DirectedGraph, vprop, eprop>       graph_t;
+
+int main(int argc, char* argv[]) {


Not a single comment in this file. Please document what the example does and maybe add some comments to the code, too.

devreal · 2019-03-13T15:10:30Z

dash/examples/ex.02.graph-deref/main.cpp

+      }
+
+      if(dash::myid() == 0) {
+        std::clock_t g_begin_time = clock();


Don't we have dash::Timer for this?

devreal · 2019-03-13T17:03:10Z

dash/include/dash/graph/internal/Graph.h

+
+namespace dash {
+
+/**


Since this is internal/Graph.h the classes in here should be put into the dash::internal namespace.

devreal · 2019-03-13T17:06:23Z

dash/include/dash/memory/GlobHeapCombinedMem.h

+   */
+  void add_globmem(glob_mem_type & glob_mem) {
+    // only GlobMem objects with the same Team can be used
+    if(*_team == glob_mem.team()) {


Should there be an error otherwise?

devreal · 2019-03-13T17:09:07Z

dash/include/dash/memory/GlobHeapCombinedMem.h

+  global_iterator          _begin;
+  global_iterator          _end;
+  local_iterator           _lbegin;
+  local_iterator           _lend;


The iterators are in an undefined state before the first commit call. Is this acceptable?

Of course not, good catch.

devreal · 2019-03-13T17:15:07Z

dash/include/dash/memory/GlobHeapContiguousLocalPtr.h

+namespace dash {
+
+// TODO: Remove this class and add template parameter for bucket list type to
+//       GlobHeapLocalPtr


Is this easy to do before merging?

devreal · 2019-03-13T17:20:39Z

dash/test/algorithm/ConnectedComponentsTest.cc

+
+TEST_F(ConnectedComponentsTest, AlgorithmRun)
+{
+  std::list<std::pair<int, int>> edge_list = {


Just out of curiosity: how was this generated (please tell me not by hand ;)) and can we generate it on the fly?

It's from a graph sample data set, @stiefn should know more.

stiefn and others added 30 commits December 12, 2016 17:26

added rough graph concept

e76743a

added empty partition concept

e9a2857

added starting point for vertex partitioning scheme concept

cbeedc9

added starting point for load balanced vertex partitioning scheem con…

3e2c0f6

…cept

renamed file

c4d54cc

changed define in Partition.h

31af333

refactored partitions and graph iterators

c4f1aa5

added raw signature for graph patterns

82d08f9

updated graph interface

6235446

added working frame for graph implementation

befde77

merged development

3bd2868

added local vertex list allocation

f155179

added local vertex iterator

162d6c6

fixed local vertex iterators

8f3dfa8

fixed seq globmem shrink

245556f

merged development

21128bb

fixed increment of LocalBucketIter

a2ba2a2

changed seq globmem functionality

dc592f0

renamed GlobDynamicSequentialMem

10594f9

added (non-working) collective allocation

091aac9

Merge branch 'development' of github.com:dash-project/dash into feat-…

465ea3a

…graph

Minor, added notes

d0d21ba

added global vertex iterator

e823a29

merged Graph.h and Graph-impl.h

a533654

added local vertex index support

da3fef7

refactored GlobDynamicContiguousMem to support multiple containers

be6abb7

added local edge iteration capabilities

93a38e6

fixed global iterators for more than one globmem container

3f2e0e9

chnaged index types

f8a109d

added alltoall dart function

d2a743c

stiefn and others added 8 commits March 8, 2018 15:37

fixed mst algorithm

cf8f89f

fixed logarithmic vertex mapper

4a78459

fixed algorithm tests

4e83239

fixed mst algorithm again

f2d31ff

added size method to wrapper function objects

e9de737

removed unnecessary code

97d12db

Merge branch 'development' of github.com:dash-project/dash into feat-…

68db7de

…graph

Updated dart_alltoallv feat-graph (missing in dev)

79bbac0

fuchsto requested review from fuerlinger, iminkov, pascalj and rkowalewski March 11, 2019 13:17

fuchsto self-assigned this Mar 11, 2019

fuchsto added feature help wanted module:dynamic-memory module:algorithms specification:dash-concepts labels Mar 11, 2019

fuchsto added 3 commits March 12, 2019 20:34

added dart_alltoallv, added graph examples, fixing compiler errors

3e6ce99

Fixed compiler errors in Graph

ec7e50f

Added graph microbench examples

8eb4a0d

Fixed clang compiler errors, added acknowledgements

1a86e36

fuchsto mentioned this pull request Mar 12, 2019

Merge and integrate dash::Graph (concepts, containers, algorithms) #637

Open

bertwesarg reviewed Mar 13, 2019

View reviewed changes

devreal requested changes Mar 13, 2019

View reviewed changes

Merge branch 'development' into feat-graph

d25cb8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Graph Concepts and Containers #634

[WIP] Graph Concepts and Containers #634

fuchsto commented Mar 9, 2019

codecov bot commented Mar 12, 2019 •

edited

Loading

fuchsto commented Mar 12, 2019

devreal commented Mar 12, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

bertwesarg Mar 13, 2019

devreal left a comment

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

fuchsto Mar 14, 2019

devreal Mar 13, 2019

devreal Mar 13, 2019

fuchsto Mar 14, 2019


		typedef dash::Graph<dash::DirectedGraph, vprop, eprop> graph_t;

		int main(int argc, char* argv[]) {

[WIP] Graph Concepts and Containers #634

Are you sure you want to change the base?

[WIP] Graph Concepts and Containers #634

Conversation

fuchsto commented Mar 9, 2019

codecov bot commented Mar 12, 2019 • edited Loading

Codecov Report

fuchsto commented Mar 12, 2019

devreal commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2019 •

edited

Loading