[REVIEW] Fix for bug in SCC on self-loops#1475
[REVIEW] Fix for bug in SCC on self-loops#1475rapids-bot[bot] merged 16 commits intorapidsai:branch-0.19from
Conversation
Merge latest release 0.17
Merge latest branch-0.18
Update forked branch-0.18
Update forked branch-0.18 from release
Update branch-0.19 from release
update forked from release branch-0.19
Merge from release branch-0.19
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #1475 +/- ##
===============================================
+ Coverage 59.04% 59.06% +0.02%
===============================================
Files 70 70
Lines 3223 3225 +2
===============================================
+ Hits 1903 1905 +2
Misses 1320 1320
Continue to review full report at Codecov.
|
BradReesWork
added
improvement
|
rerun tests |
rlratzel
left a comment
rlratzel
left a comment
There was a problem hiding this comment.
I have two minor comments that need not hold up approval if they don't get addressed.
| } else { | ||
| SCC_Data<ByteT, VT> sccd(nrows, graph.offsets, graph.indices); | ||
| sccd.run_scc(labels); | ||
| auto num_iters = sccd.run_scc(labels); |
There was a problem hiding this comment.
Is this an unused variable, and if so should it be removed? I don't see it anywhere else in the diff.
There was a problem hiding this comment.
It's useful for debugging and harmless. If you build debug and put a breakpoint there, you immediately get the num_iters, as opposed to having to re-build to provide it. It's not for immediate consumption, but for debugging needs. Does not trigger a warning, so I'd leave it there.
| std::vector<IndexT> cooRowInd{0, 0, 1, 1, 2, 2, 3, 3, 4}; | ||
| std::vector<IndexT> cooColInd{0, 1, 0, 1, 0, 2, 1, 3, 4}; |
There was a problem hiding this comment.
Thanks for adding the additional tests. I think a FIXME could be added to change these new tests to be parameterized, with input parameters being nrows, the COO col and row indices, and the expected labels and number of components. That would reduce a lot of redundant code and make adding new graphs to test easier.
There was a problem hiding this comment.
I'm not sure what is the desired course of action here. A FIXME saying what? Remember this code path is to be retired (soon). So there's a way bigger FIXME looming here. This would add confusion, I think.
|
@gpucibot merge |
4 participants
This provides fixes for strongly connected components on graphs with self-loops: #1471.
closes #1471