[REVIEW] Add support for shortest_path_length and fix graph vertex checks#1278
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
ok to test |
BradReesWork
added
the
improvement
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
|
|
||
| # Step 5: Check if source index is valid | ||
| if not 0 <= source < num_verts: | ||
| if source is cudf.NA or 0 > source or num_verts <= source: |
There was a problem hiding this comment.
It might be cleaner to raise an exception for source is cudf.NA in sssp.py after doing the renumbering conversion (if necessary). I usually think of the algorithm.py file as dealing with things related to getting into the context for the calling C++ and the algorithm_wrapper.pyx file as dealing with the mechanical aspects of calling C++.
afender
left a comment
afender
left a comment
There was a problem hiding this comment.
Looks good. Thank you!
I just added a minor suggestion.
|
|
||
|
|
||
| # TODO: need to add docs, test with cupy matrix and networkx graphs | ||
| def shortest_path_length(G, source, target=None): |
There was a problem hiding this comment.
In addition to docs, please consider transferring this function to sssp.py for consistency
jpurviance
force-pushed
the
shortest-path-length
branch
from
a9bce48 to
574b8a3
Compare
jpurviance
changed the title
|
Ready for review |
|
I've been looking over the contributing.md docs. I noticed that there is a changelog step. is this completed for every pr? Should I take the effort to complete that now or after the PR if so. Also, I noticed there is collection of |
We're switching to auto-generate change-log using PR title and message (this should happen very soon), but before then, CHANGELOG should be manually updated. Yes, the authors are expected to add [WIP] (work-in-progress) or [REVIEW] (ready-to-be-reviewed)in the PR title to indicate the current state of the PR. |
BradReesWork
changed the title
|
@jpurviance code looks good, just need the CHANGELOG updated.. As Seunghwa mentioned above, that step is going to be dropped as we move to automatically updating teh change log based on PR title, but that is st ill a few weeks away |
jpurviance
changed the title
afender
left a comment
afender
left a comment
There was a problem hiding this comment.
Thank you, John!
I noticed there's a small merge conflict in sssp.py as well as the changelog check that is still complaining.. We should be able to press the merge button once this is addressed.
jpurviance
force-pushed
the
shortest-path-length
branch
from
574b8a3 to
a206b5d
Compare
jpurviance
force-pushed
the
shortest-path-length
branch
from
a206b5d to
2dd0172
Compare
|
Change log updated and merge conflicts resolved. |
Codecov Report
@@ Coverage Diff @@
## branch-0.17 #1278 +/- ##
===============================================
+ Coverage 60.07% 60.38% +0.30%
===============================================
Files 67 67
Lines 3006 3029 +23
===============================================
+ Hits 1806 1829 +23
Misses 1200 1200
Continue to review full report at Codecov.
|
|
@jpurviance thanks for diving in and helping make cuGraph better |
7 participants
Adds the ability to get the length/cost of path from source to destination(s). Closely follows
networkx.shortest_path_length.Similarities:
cudfdataframe is returned with columns:[vertex, distance](similar tonetworkxdictionary return)sys.float_info.maxif the vertex is not reachable.Differences:
networkxFixes:
ssspandcugraph.Graph.has_nodevertex checks. Added support for checking for vertexes that are not apart of the graph. in the past,TypeErrorwas raised when doing a comparison check (as apposed toValueError)