Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate edge weights from graph objects and update primitives to support general edge properties. #2843

Merged
merged 41 commits into from
Nov 21, 2022

Conversation

seunghwak
Copy link
Contributor

@seunghwak seunghwak commented Oct 24, 2022

This PR separates edge weights from graph_t and graph_view_t. Now edge weights are stored in edge_property_t like any other edge properties (e.g. edge IDs & types).

Two major benefits are

  1. For algorithms working on unweighted graphs, we don't need to explicitly instantiate for weight_t = float and weight_t = double cases. This saves compile time & reduces the binary size.
  2. Primitives can work with different edge properties (weights, IDs, types, and so on) using a single mechanism instead of using one mechanism for edge weights and another mechanism for other edge properties (e.g. edge IDs & types).

This PR changes many files (due to the changes in the graph_t and graph_view_t classes) but most changes are repetitive.

To highlight major changes,

template <typename vertex_t, typename edge_t, typename weight_t, bool store_transposed, bool multi_gpu>
class graph_(view_)t {
...
}
=>
template <typename vertex_t, typename edge_t, bool store_transposed, bool multi_gpu>
class graph_(view_)t {
...
}

create_graph_from_edgelist will return

std::tuple<
  graph_t<vertex_t, edge_t, store_transposed, multi_gpu>,
  std::optional<
    edge_property_t<graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu>, weight_t>>,
  std::optional<edge_property_t<graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu>,
                                thrust::tuple<edge_t, edge_type_t>>>,
  std::optional<rmm::device_uvector<vertex_t>>>

instead of

std::tuple<
  graph_t<vertex_t, edge_t, weight_t, store_transposed, multi_gpu>,
  std::optional<edge_property_t<graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu>,
                                thrust::tuple<edge_t, edge_type_t>>>,
  std::optional<rmm::device_uvector<vertex_t>>>

i.e. edge weights will be returned in a separate object.

Algorithms working on weighted graphs (or both unweighted & weighted graphs) will take the additional input argument:
edge_property_view_t<edge_t, weight_t const*> or std::optional<edge_property_view_t<edge_t, weight_t const*>>

Primitives e_op's input parameters were previously (vertex_t src, vertex_t dst, weight_t w, source_property, destination_property) or (vertex_t src, vertex_t dst, source_property, destination_property).
Now it will be (vertex_t src, vertex_t dst, source_property, destination property, edge property)
Primitives with e_op will take an additional edge_property_view_t type input parameter (which can be edge_dummy_property_t{}.view() if no edge properties are used).

Edge weights are added in the C++ side (before type erasing) of the C API cugraph_graph_t.

struct cugraph_graph_t {
...
  void* edge_weights_;  // this is added.
...
}

This doesn't require any changes in the C side of the C API, so I assume this won't disrupt the pylibcugraph and python layers (once cython.cu file is removed).

@seunghwak seunghwak requested review from a team as code owners October 24, 2022 18:49
@seunghwak seunghwak self-assigned this Oct 24, 2022
@seunghwak seunghwak added feature request New feature or request 2 - In Progress breaking Breaking change labels Oct 24, 2022
@seunghwak seunghwak added this to the 22.12 milestone Oct 24, 2022
@seunghwak seunghwak added 3 - Ready for Review DO NOT MERGE Hold off on merging; see PR for details and removed 2 - In Progress labels Oct 31, 2022
@seunghwak seunghwak changed the title [WIP][skip-ci] Separate edge weights from graph objects and update primitives to support general edge properties. [skip-ci] Separate edge weights from graph objects and update primitives to support general edge properties. Oct 31, 2022
@BradReesWork BradReesWork added the Blocked Cannot progress due to external reasons label Nov 1, 2022
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this simplifies much of the code - especially when we don't operate on weights.

Looks good. There are several PRs in the pipeline that will merge before this that will require updates based on the changes here, but it should be relatively manageable.

@seunghwak seunghwak changed the title [skip-ci] Separate edge weights from graph objects and update primitives to support general edge properties. Separate edge weights from graph objects and update primitives to support general edge properties. Nov 18, 2022
@seunghwak seunghwak removed Blocked Cannot progress due to external reasons DO NOT MERGE Hold off on merging; see PR for details labels Nov 18, 2022
@seunghwak
Copy link
Contributor Author

rerun tests

@seunghwak
Copy link
Contributor Author

rerun tests

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@8946e1c). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.12    #2843   +/-   ##
===============================================
  Coverage                ?   60.10%           
===============================================
  Files                   ?      123           
  Lines                   ?     7277           
  Branches                ?        0           
===============================================
  Hits                    ?     4374           
  Misses                  ?     2903           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4b7d4fb into rapidsai:branch-22.12 Nov 21, 2022
@seunghwak seunghwak deleted the fea_prim_edge_property branch November 30, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants