Refactor the python function symmetrizing the edgelist #4649

jnke2016 · 2024-09-10T13:26:22Z

This PR updates the python API to symmetrize the edge list through the CAPI for PLC algorithms. This PR also deprecates the legacy python function symmetrizing the edge list

closes #4588

…0_refactor_symmetrize

seunghwak

Reviewed the C part, LGTM (besides one question)

seunghwak · 2024-09-24T17:30:29Z

cpp/include/cugraph_c/graph_functions.h

+                                         const cugraph_type_erased_device_array_view_t* weights,
+                                         bool_t reciprocal,
+                                         cugraph_induced_subgraph_result_t** result,
+                                         cugraph_error_t** error);


Aren't we planning to remove this function from the python API? Should we expose this in the C-API?

Agreed, I thought we decided this was not going to be exposed via the C API.

ChuckHastings

A few things to address.

ChuckHastings · 2024-09-24T19:20:50Z

cpp/include/cugraph_c/graph_functions.h

+                                         const cugraph_type_erased_device_array_view_t* weights,
+                                         bool_t reciprocal,
+                                         cugraph_induced_subgraph_result_t** result,
+                                         cugraph_error_t** error);


Agreed, I thought we decided this was not going to be exposed via the C API.

ChuckHastings · 2024-09-24T19:26:40Z

cpp/include/cugraph_c/graph.h

@@ -150,6 +152,9 @@ cugraph_error_code_t cugraph_graph_create_sg(
 *    If false, do not renumber.  Renumbering enables some significant optimizations within
 *    the graph primitives library, so it is strongly encouraged.  Renumbering is required if
 *    the vertices are not sequential integer values from 0 to num_vertices.
+ * @param [in]  symmetrize     If true, symmetrize the edgelist.
+ *    or take the maximum weight), the caller should remove specific edges themselves and not rely


This comment doesn't read correctly, looks like we didn't get the entire comment from wherever it was copied.

We should add in the assumption that only weights can be symmetrized currently... if you have edge_ids or edge_types you can't specify symmetrize as true.

I removed the unrelated comments. I also update the doc strings to reflect that the symmetrization of edges with edge and type ids is currently not supported. In the CAPI, I return an unsupported error if either or is specified

ChuckHastings · 2024-09-24T19:26:57Z

cpp/include/cugraph_c/graph.h

@@ -190,6 +196,9 @@ cugraph_error_code_t cugraph_sg_graph_create_from_csr(
 *    If false, do not renumber.  Renumbering enables some significant optimizations within
 *    the graph primitives library, so it is strongly encouraged.  Renumbering is required if
 *    the vertices are not sequential integer values from 0 to num_vertices.
+ * @param [in]  symmetrize     If true, symmetrize the edgelist.


Same as above.

ChuckHastings · 2024-09-24T19:31:31Z

cpp/src/c_api/graph_sg.cpp

@@ -207,6 +210,22 @@ struct create_graph_functor : public cugraph::c_api::abstract_functor {
                                        : false);
      }

+      if (symmetrize_) {


In addition to this logic, if symmetrize_ is true then we should override the graph properties to set is_symmetric to true.

ChuckHastings · 2024-09-24T19:31:49Z

cpp/src/c_api/graph_sg.cpp

@@ -398,6 +420,22 @@ struct create_graph_csr_functor : public cugraph::c_api::abstract_functor {
        cugraph::graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu>,
        edge_type_id_t>(handle_);

+      if (symmetrize_) {


Same comment as above regarding is_symmetric

ChuckHastings · 2024-09-24T19:32:22Z

cpp/src/c_api/graph_sg.cpp

@@ -398,6 +420,22 @@ struct create_graph_csr_functor : public cugraph::c_api::abstract_functor {
        cugraph::graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu>,
        edge_type_id_t>(handle_);

+      if (symmetrize_) {
+        if (edgelist_edge_ids || edgelist_edge_types) {


We should add an issue to discuss what symmetrization means with respect to edge properties.

I tracked this in an issue

jnke2016 · 2024-09-24T19:35:55Z

Agreed, I thought we decided this was not going to be exposed via the C API.

Right we are not exposing it to the python API. I forgot to remove it from the CAPI as well

rlratzel

Thanks! Just a few requests below.

rlratzel · 2024-09-29T20:21:49Z

python/cugraph/cugraph/components/connectivity.py

+    print("G = ", G)
+


rlratzel · 2024-09-29T20:28:44Z

python/cugraph/cugraph/structure/graph_classes.py

+            behavior symmetrizes the edges if the graph is undirected. If the edges
+            are already symmetric, set this flag to False.


Should you also mention the restriction on edgelists with edge IDs and edge types? Also, the way this is worded makes it seem like the user must figure out if the edgrs are symmetric or not and set the flag accordingly. Maybe it would be better if it was worded like this:

Suggested change

behavior symmetrizes the edges if the graph is undirected. If the edges

are already symmetric, set this flag to False.

behavior symmetrizes the edges if the graph is undirected. This flag cannot be set to True if the edgelist contains edge IDs or edge Types. If the incoming edgelist is intended for an undirected graph and it is known to be symmetric, this flag can be set to False to skip the symmetrization step for better performance.

Same suggestion for from_cudf_adjlist below.

Got it. I reworded it as you suggested

rlratzel · 2024-09-29T20:34:59Z

python/cugraph/cugraph/utilities/nx_factory.py

+        print("creating a directed graph")
        G = cugraph.Graph(directed=True)
    elif isinstance(nxG, nx.classes.graph.Graph):
+        print("creating an undirected graph")


more leftover debug prints.

…0_refactor_symmetrize

rlratzel

Thanks, almost done, still see a debug print and I had one minor request.

python/cugraph/cugraph/structure/graph_implementation/simpleGraph.py

python/cugraph/cugraph/tests/structure/test_multigraph.py

rlratzel · 2024-10-02T16:43:36Z

/merge

add new symmetrize file

f2f3a04

jnke2016 requested a review from a team as a code owner September 10, 2024 13:26

github-actions bot added the cuGraph label Sep 10, 2024

implement symmetrize_edgelist in the CAPI

0c8b927

jnke2016 requested a review from a team as a code owner September 10, 2024 14:52

github-actions bot added the CMake label Sep 10, 2024

jnke2016 changed the title ~~Refactor the python function symmetrizing the edgeless~~ Refactor the python function symmetrizing the edgelist Sep 11, 2024

jnke2016 marked this pull request as draft September 11, 2024 15:16

jnke2016 added 3 commits September 19, 2024 07:06

deprecated the python symmetrize function

c01456a

expose flag symmetrizing the edgelist

fb115da

Remove python symmetrize from the SG graph creation

47b0677

github-actions bot added the python label Sep 22, 2024

update API to symmetrize the edgelist and update docstring

f09fd41

github-actions bot removed the CMake label Sep 23, 2024

jnke2016 added 5 commits September 22, 2024 19:08

deprecate python method to symmetrize the edgelist

b737b02

fix style

ea19b62

fix style

bd7eede

Merge remote-tracking branch 'upstream/branch-24.10' into branch-24.1…

6e433ab

…0_refactor_symmetrize

fix typo

e5c94f7

jnke2016 marked this pull request as ready for review September 23, 2024 13:46

jnke2016 requested a review from a team as a code owner September 23, 2024 13:46

jnke2016 self-assigned this Sep 23, 2024

seunghwak approved these changes Sep 24, 2024

View reviewed changes

ChuckHastings reviewed Sep 24, 2024

View reviewed changes

jnke2016 added 5 commits September 26, 2024 08:46

update docstrings and remove unused function declaration

0470299

ensure the graph properties match when symmetrizing

2df98f5

update docstrings

fabcb37

update docstrings

3b1c1d0

update error reporting

0462529

jnke2016 added 4 commits September 28, 2024 16:26

support the symmetrization of edgelist for legacy algorithms

e8beb72

fix typo

e3e413b

fix property_graph tests

5336a91

update graph creation for the C tests

3b40fc5

rlratzel requested changes Sep 29, 2024

View reviewed changes

jnke2016 added 10 commits September 30, 2024 17:17

update the test to call the deprecated symmetrize

cbbfb88

fix typo

8891b1f

fix typo

4e9e85d

undo changes to symmetrize

d135ac1

fix typo

a9c7064

use the deprecated symmetrize call

1c70046

update SG graph creation

2cfb0fb

Merge remote-tracking branch 'upstream/branch-24.10' into branch-24.1…

fec091f

…0_refactor_symmetrize

fix style

efbdb38

fix style

35c25db

rlratzel requested changes Oct 2, 2024

View reviewed changes

python/cugraph/cugraph/structure/graph_implementation/simpleGraph.py Outdated Show resolved Hide resolved

python/cugraph/cugraph/tests/structure/test_multigraph.py Outdated Show resolved Hide resolved

jnke2016 added 6 commits October 1, 2024 22:40

remove debug print and pass kwarg name

f91bb81

call deprecated symmetrize call

0389f27

update flag passed to the deprecated symmetrize function

97472cd

fix style

ec7da8d

update docstrings

0e5a9ba

remove redundant assignments

2a78356

rlratzel approved these changes Oct 2, 2024

View reviewed changes

rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 2, 2024

jnke2016 added 3 commits October 2, 2024 09:44

ignore deprecation warning for symmetrizing

43ba0cd

symmetrize

cfecd90

fix style

96d2e20

raydouglass merged commit 5fad435 into rapidsai:branch-24.10 Oct 3, 2024
130 of 131 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the python function symmetrizing the edgelist #4649

Refactor the python function symmetrizing the edgelist #4649

jnke2016 commented Sep 10, 2024 •

edited

Loading

seunghwak left a comment

seunghwak Sep 24, 2024

ChuckHastings Sep 24, 2024

ChuckHastings left a comment

ChuckHastings Sep 24, 2024

ChuckHastings Sep 24, 2024

jnke2016 Sep 26, 2024

ChuckHastings Sep 24, 2024

ChuckHastings Sep 24, 2024

ChuckHastings Sep 24, 2024

ChuckHastings Sep 24, 2024

jnke2016 Sep 26, 2024

jnke2016 commented Sep 24, 2024

rlratzel left a comment

rlratzel Sep 29, 2024

rlratzel Sep 29, 2024

jnke2016 Oct 2, 2024

rlratzel Sep 29, 2024

rlratzel left a comment

rlratzel commented Oct 2, 2024

		behavior symmetrizes the edges if the graph is undirected. If the edges
		are already symmetric, set this flag to False.

Refactor the python function symmetrizing the edgelist #4649

Refactor the python function symmetrizing the edgelist #4649

Conversation

jnke2016 commented Sep 10, 2024 • edited Loading

seunghwak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuckHastings 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

jnke2016 commented Sep 24, 2024

rlratzel 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

rlratzel left a comment

Choose a reason for hiding this comment

rlratzel commented Oct 2, 2024

jnke2016 commented Sep 10, 2024 •

edited

Loading