Remove major/minor from renumber_edgelist public functions. #2116
Conversation
seunghwak
added
3 - Ready for Review
improvement
| @@ -186,13 +186,18 @@ def renumber(input_df, # maybe use cpdef ? | |||
| shuffled_df = input_df | |||
| edge_counts_32 = make_unique[vector[int]](1, num_local_edges) | |||
|
|
|||
| shuffled_major = major_vertices.__cuda_array_interface__['data'][0] | |||
| shuffled_minor = minor_vertices.__cuda_array_interface__['data'][0] | |||
| if not transposed: | |||
There was a problem hiding this comment.
checking transposed again makes it redundant since it is already done in 179. I believe idea was just to swap the column name which will swap the src column and dst column if transpose=True. However, unless I am misunderstanding something, major_vertices and minor_vertices should be assigned shuffled_df column after checking for transposed (not related to this PR). If not , when transpose=True major_vertices and minor_vertices do not get update. Unless major_vertices and minor_vertices point to the same address as shuffled_df columns which I doubt.
There was a problem hiding this comment.
After looking at the whole code . The transpose check is already done in line 88, so this change will be doing the reverse( ignore my comment above)
rlratzel
dismissed
their stale review
@jnke2016 pointed out something I may have overlooked, so I'm re-reviewing.
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #2116 +/- ##
===============================================
Coverage ? 73.99%
===============================================
Files ? 157
Lines ? 10496
Branches ? 0
===============================================
Hits ? 7767
Misses ? 2729
Partials ? 0 Continue to review full report at Codecov.
|
|
@gpucibot merge |
Partially addresses #2003
We should not use major,minor (instead of src, dst) in the public non-detail API and should use major,minor in the detail space.
The public non-detail namespace renumber_edgelist currently uses major, minor and this PR fixes this.
This PR also replaces row/col to src/dst in
single_gpu_renumber_edgelist_given_number_mapas we're aiming to consistently use src/dst instead of row/col in our public C++ API.This is possibly breaking as this PR changes the public API (but I am not aware of any non-cugraph libraries directly calling renumber_edgelist).