ericmjl
commented
5 years ago I would like to mirror the comments by @seunghwak. In addition, coming in as a NetworkX user, I was also surprised by the API design. Being surprised by the API is a red flag for me as an end-user, and a tutorial leader for NetworkX-based tutorials (I have a number coming up over the next few months).
Having seen a tweet on Twitter, I had quite a number of high hopes regarding the API. In particular, because NetworkX has been so idiomatic for learning and doing graph analytics, I thought it would be super awesome if cuGraph just outright copied the NetworkX API, and the team followed-up with PRs to NetworkX to suggest improvements to the NetworkX API (and sync it up) where the nx API might not be the best. In particular, if the API were switched over to explicitly mirror the NetworkX API, I could very easily recommend using it as the solution for scalability when I do my NetworkX tutorials at conferences. Scaling up is a common question for NetworkX users, and I have traditionally recommended iGraph with some hesitation, because the API is different. As it stands, however, cuGraph is not a solution I would be ready to recommend - the API is very different from NetworkX's API.
Graph creation, in particular, is a great example of where I was surprised when I read the cuGraph API. Reading the docs is when I realized that cuGraph is instead not a drop-in replacement for NetworkX.
In NetworkX, the following API calls are used:
import networkx as nx
G = nx.Graph()
G.add_edges_from(...) # add a set of edges to the graph
G.add_edge(...) # add an edge to the graph
G.add_nodes_from(...) # add a set of nodes to the graph
G.add_node(...) # add a node
G.nodes # get view of nodes
G.edges # get view of edgesIn order to make cuGraph a drop-in replacement for NetworkX, at the very minimum, those same class methods should be present.
In addition, in NetworkX, "edge list" is the term used for a list of two- or three-tuples that are passed in to create edges, with nodes automatically created. "adjacency", from my memory, as a term it is reserved the adjacency matrix. This separation of vocabulary makes it much easier for an end-user to reason about what exactly we're requesting for from the graph.
I'm happy to chat further, and even help out with input on where to match the API for end-users. The underlying implementation is one for which I'm less concerned about (though important, for reproducibility/accuracy/"science etc."), but the API really matters.
The RAPIDS team has done a great job already with cuDF copying the pandas API, and CuPy also has been awesome with its explicit "copy NumPy" policy. cuGraph actually copying NetworkX's API would make this a super awesome project too!
seunghwak
Iroy30
BradReesWork
Is your feature request related to a problem? Please describe. Related: https://github.com/rapidsai/cugraph/issues/187
class Graph (defined in c_graph.pyx) stores a graph and provides functions to initialize, delete, convert (between different graph formats), and query.
There are corresponding C++/CUDA level functions.
There exist some inconsistencies in naming, and some functions may surprise users. In general, surprising users is bad in API design. If a user expects a function to do A, but the function silently does B, or (B + A), it may leads to a hard-to-fix bug till the user finds that the function is doing something unexpected.
So inconsistencies. In the python level, add_edge_list and add_adj_list initialize a graph while add_transposed_adj_list performs conversion. In CUDA/C++, gdfadd..._list performs conversion but add_edge_list and add_adjlist perform initialization in python. In CUDA/C++, gdf..._list_view functions perform initialization. Calling both add_edge_list and add_adj_list can result in a single graph instance storing two different graphs (issue #187). And we have num_vertices, why no num_edges?
And the surprise part. view_edge_list, view_adj_list, and num_vertices implicitly perform conversion. This requires more thoughts as there is a design trade-off. Implicit (or automatic) conversion allows users to type less. But this can require a significant amount of computing and also more memory for large graphs. It can be quite surprising for users to see memory allocation error after long wait when they simply query the number of vertices.
Describe the solution you'd like So we may clarify the API functions for initialization, deletion, conversion, and query and also need to fix inconsistencies between the python API and C++/CUDA API.
For initialization:
For deletion: What we have looks OK to me...
For conversion:
For query:
Describe alternatives you've considered Any comments and thoughts (and new options) will be appreciated.