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

[REVIEW] Bug ext two graphs #239

Merged
merged 14 commits into from
Apr 24, 2019
Merged

Conversation

seunghwak
Copy link
Contributor

Close #187

…rkX has number_of_nodes, so the new name is closer to the NetworkX name, we haven't decided to use either node or vertex in the user facing API yet, so we stick to vertex up to this point, both num_vert and num_verts are used, and renamed num_vert to num_verts, cuGraph mostly uses vertex but node is used in several places, so renamed node(s) to vertex (or vertices) to improve consistency)
…tructure to get the number of edges (e.g. g.adjList.indices.size) with the number_of_edges() function call
…a GDFError (GDF_INVALID_API_CALL) if this condition is not met.
@seunghwak seunghwak marked this pull request as ready for review April 23, 2019 20:41
@BradReesWork BradReesWork changed the title [WIP] Bug ext two graphs [REVIEW] Bug ext two graphs Apr 23, 2019
//representation to prevent a single object storing two different graphs.
GDF_REQUIRE( ((graph->edgeList == nullptr) && (graph->adjList == nullptr) &&
(graph->transposedAdjList == nullptr)), GDF_INVALID_API_CALL);
GDF_REQUIRE( (graph->transposedAdjList == nullptr) , GDF_INVALID_API_CALL);
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding this and I made a fix!

else:
elif g.edgeList:
# This code needs to be revisited when updating gdf_graph. Users
# may expect numbrer_of_vertcies() as a cheap query but this
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I created #241 for this. I don't expect many users to upload an edge list and directly ask for the number of vertices before actually running any analytics. So it would be nice to have but I don't see it as a top priority.

Copy link
Contributor

@jwyles jwyles left a comment

Choose a reason for hiding this comment

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

Looks good.

@afender afender merged commit 40caa93 into rapidsai:branch-0.7 Apr 24, 2019
@seunghwak seunghwak deleted the bug_ext_two_graphs branch May 16, 2019 20:49
ChuckHastings pushed a commit to ChuckHastings/cugraph that referenced this pull request Jun 14, 2021
[gpuCI] Forward-merge branch-21.06 to branch-21.08 [skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants