Nothing Special   »   [go: up one dir, main page]

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

extract the edgelist from the graph #4750

Open
wants to merge 22 commits into
base: branch-24.12
Choose a base branch
from

Conversation

jnke2016
Copy link
Contributor
@jnke2016 jnke2016 commented Nov 9, 2024

This PR exposes the C++ function decompress_to_edgelist to the C, PLC and Python API. This will enable the extraction of the edgelist from a graph which is currently not supported. It also removes the deprecated parameter legacy_renum_only

@jnke2016 jnke2016 requested review from a team as code owners November 9, 2024 09:35
@jnke2016 jnke2016 changed the title extract edgelist from the graph extract the edgelist from the graph Nov 9, 2024
@jnke2016 jnke2016 self-assigned this Nov 15, 2024
@jnke2016 jnke2016 added this to the 24.12 milestone Nov 15, 2024
@@ -979,6 +980,83 @@ def convert_to_cudf(cp_arrays):

return ddf

def decompress_to_edgelist(
self,
original: bool = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be more descriptive if it was named return_unrenumbered_edgelist


Returns
-------
ego_edge_lists : dask_cudf.DataFrame
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right name for the return value? Also, should it be plural? I figured it would be just edgelist

Comment on lines 96 to 98
>>> (sources, destinations, edge_weights, _, _) =
... pylibcugraph.induced_subgraph(
... resource_handle, G, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be calling decompress_to_edgelist somewhere?

cugraph_error_code_t cugraph_decompress_to_edgelist(const cugraph_resource_handle_t* handle,
cugraph_graph_t* graph,
bool_t do_expensive_check,
cugraph_induced_subgraph_result_t** result,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion, but cugraph_induced_subgraph_result_t sounds a bit of misnomer here. We may used this data structure to just store edge list from the induced subgraph algorithm in the past, but if we wish to use this for other algorithms as well, we may better rename this now or sometime in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. I would suggest cugraph_edgelist_t. I have a draft PR (that I need to discard and start over with) that was going to create a cugraph_edgelist_t for the C API for graph construction.

I'd suggest the following:

  • Replicate the cugraph_induced_subgraph_result_t type and C API accessor functions to call it cugraph_edgelist_t instead
  • Mark the induced subgraph variations as deprecated
  • Within the C API implementation (src/c_api) you can either rename the cugraph::c_api::cugraph_induced_subgraph_result_t to cugraph::c_api::cugraph_edgelist_t and rename all references accordingly, or you can leave it using the legacy version and I'll fix it in 25.02 when I finish the C API graph construction updates.

transpose_storage<vertex_t, edge_t, weight_t, store_transposed, multi_gpu>(
handle_, graph_, error_.get());
if (error_code_ != CUGRAPH_SUCCESS)
;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants