Group return values of renumber_edgelist and input parameters of graph_t & graph_view_t constructors.#1816
Conversation
…(view_)t constructors to better group meta information
seunghwak
added
3 - Ready for Review
improvement
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #1816 +/- ##
================================================
+ Coverage 59.85% 69.56% +9.71%
================================================
Files 77 137 +60
Lines 3547 8579 +5032
================================================
+ Hits 2123 5968 +3845
- Misses 1424 2611 +1187 Continue to review full report at Codecov.
|
|
|
||
| } // namespace detail | ||
|
|
||
| template <typename vertex_t, typename edge_t, bool multi_gpu, typename Enable = void> |
There was a problem hiding this comment.
graph_view_meta_t seems repetitive of a functionality that graph_meta_t already offers. Is there a strong motivation for this apparent code duplication? Would it be possible to delegate this grouping functionality to only one such meta pair?
There was a problem hiding this comment.
Yes, they are currently pretty much the same but this restructuring is mainly to pass additional parameters. When we support (key, value) pairs for row/column properties, pointers to unique rows/columns will be only passed to graph_view_meta_t.
So, these two will diverge (and possibly diverge even more over time), and that's the key motivation for having two separate classes.
There was a problem hiding this comment.
@seunghwak will graph_meta_t always be a subset of graph_view_meta_t (e.g. derived class) or will the two be completely unique over time?
There was a problem hiding this comment.
There will be an overlap but the two will have non-overlapping parts.
| minor_ptrs, | ||
| edgelist_edge_counts, | ||
| edgelist_intra_partition_segment_offsets); | ||
| std::tie(renumber_map_labels, meta) = cugraph::renumber_edgelist<vertex_t, edge_t, multi_gpu>( |
There was a problem hiding this comment.
Perhaps time to progressively replace std::tie() by C++17 structural bindings, which might be a bit more readable?
There was a problem hiding this comment.
Yeah... Here I used std::tie instead of structural binding as renumber_map_labels & meta are defined outside the {} block, but I don't see much need for this block (besides, releasing the host memory hold by std::vector<vertex_t const*> major_ptrs & minor_ptrs little early... I will update this.
| store_transposed ? edgelist_cols.data() : edgelist_rows.data(), | ||
| store_transposed ? edgelist_rows.data() : edgelist_cols.data(), | ||
| static_cast<edge_t>(edgelist_rows.size())); | ||
| std::tie(*renumber_map_labels, meta) = cugraph::renumber_edgelist<vertex_t, edge_t, multi_gpu>( |
There was a problem hiding this comment.
Same comment on std::tie() vs. structural bindings.
There was a problem hiding this comment.
This is not possible because renumber_map_labels & meta are defined outside the if (renumber) {} block.
|
@gpucibot merge |
5 participants
Group return values of
renumber_edgelist(except for the main renumber_map) and return as a renumber_meta_t object.Group input parameters of
graph_t&graph_view_tconstructors (except for the raft::handle and input parameters for the actual graph data) as a single graph_(view_)meta_t variable.This is to make it easier to add more meta data in the future.