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] 2D cython/python infrastructure- BFS & SSSP #1177

Merged
merged 85 commits into from
Oct 13, 2020

Conversation

Iroy30
Copy link
Contributor

@Iroy30 Iroy30 commented Sep 30, 2020

No description provided.

@Iroy30 Iroy30 requested a review from a team as a code owner September 30, 2020 21:02
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@Iroy30 Iroy30 changed the title [WIP] 2D infra- bfs and sssp [WIP] 2D cython/python infrastructure- BFS & SSSP Sep 30, 2020
@BradReesWork
Copy link
Member

rerun tests

@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #1177 into branch-0.16 will decrease coverage by 0.24%.
The diff coverage is 32.55%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.16    #1177      +/-   ##
===============================================
- Coverage        57.04%   56.79%   -0.25%     
===============================================
  Files               61       62       +1     
  Lines             2514     2546      +32     
===============================================
+ Hits              1434     1446      +12     
- Misses            1080     1100      +20     
Impacted Files Coverage Δ
python/cugraph/dask/link_analysis/pagerank.py 25.80% <ø> (ø)
python/cugraph/dask/traversal/bfs.py 32.14% <28.57%> (+4.14%) ⬆️
python/cugraph/dask/traversal/sssp.py 32.14% <32.14%> (ø)
python/cugraph/dask/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95be085...9f734a8. Read the comment docs.

dg = cugraph.DiGraph()
dg.from_dask_cudf_edgelist(ddf, "src", "dst")

expected_dist = cugraph.sssp(g, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Second problem looks similar to the issue that was fixed with pagerank values... note that the index restarts... perhaps the predecessor and distance aren't getting gathered from the correct GPU?

graph_container.graph_ptr_union.GraphCSCViewDoublePtr->get_vertex_identifiers(
reinterpret_cast<int32_t*>(identifiers));
} else if (graph_container.graph_type == graphTypeEnum::graph_t) {
if (graph_container.edgeType == numberTypeEnum::int32Type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be cleaner (for pagerank, BFS and SSSP) to use the technique used by louvain... define a functor for each algorithm and then call detail::call_function<true, void>(handle, graph_container, functor);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although we could defer this work to 0.17

Copy link
Member

@afender afender 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, but some conflicts here as well.

graph : cugraph.DiGraph
cuGraph graph descriptor, should contain the connectivity information
as dask cudf edge list dataframe(edge weights are not used for this
algorithm). Undirected Graph not currently supported.
Copy link
Member

Choose a reason for hiding this comment

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

"Undirected Graph not currently supported."
Could we add that undirected graphs should be passed as Digraph with edges in both directions (apply to other MG APIs as well).


Returns
-------
df : cudf.DataFrame
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a dask data frame or are we assuming that 3 global V sets will fit in one GPU?

Copy link
Member

Choose a reason for hiding this comment

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

Saw that in #1175 too

@BradReesWork BradReesWork merged commit b704d13 into rapidsai:branch-0.16 Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants