Fix OOB error, BFS C API should validate that the source vertex is a valid vertex #4077
Conversation
ChuckHastings
added
bug
cpp/src/detail/utility_wrappers.cu
Outdated
| size_t local_count = thrust::count_if( | ||
| handle.get_thrust_policy(), span.begin(), span.end(), [value] __device__(data_t d) { | ||
| return d == value; | ||
| }); |
There was a problem hiding this comment.
You can use thrust::count (https://thrust.github.io/doc/group__counting_ga4f2955d87ada73fc89f3442a19ca1bfa.html#ga4f2955d87ada73fc89f3442a19ca1bfa) and no need to provide a predicate.
cpp/src/detail/utility_wrappers.cu
Outdated
| if constexpr (multi_gpu) { | ||
| local_count = cugraph::host_scalar_allreduce( | ||
| handle.get_comms(), local_count, raft::comms::op_t::SUM, handle.get_stream()); | ||
| } |
There was a problem hiding this comment.
All the other functions in this file are SG functions, and this function is an MG function. This might be confusing...
Should we better resolve this?
- No.
- Create another file for MG utility wrapper functions
- Just call thrust::count() and host_scalar_allreduce() instead of creating this function.
- Rename functions to better hint that this function performs allerduce if MG
- Something else?
What do you think?
There was a problem hiding this comment.
I'll look at option 2.
While I like option 3, there are many cases where we don't need to create a .cu file except for common utilities like this. This allows us to leave many files as .cpp files instead.
There was a problem hiding this comment.
Actually, I think what I'll do is move the all reduce (which doesn't use thrust, so can be called in a .cpp file) out of the function, making it an SG function. So I'll leave it here but remove the multi_gpu flag.
…l case testing and skipped tests from prior PR needed due to the bug this PR fixes.
| # Individually run tests that are skipped above b/c they may run out of memory | ||
| PYTEST_NO_SKIP=True ./run_nx_tests.sh --cov-append -k "TestDAG and test_antichains" | ||
| PYTEST_NO_SKIP=True ./run_nx_tests.sh --cov-append -k "TestMultiDiGraph_DAGLCA and test_all_pairs_lca_pairs_without_lca" | ||
| PYTEST_NO_SKIP=True ./run_nx_tests.sh --cov-append -k "TestDAGLCA and test_all_pairs_lca_pairs_without_lca" | ||
| PYTEST_NO_SKIP=True ./run_nx_tests.sh --cov-append -k "TestEfficiency and test_using_ego_graph" |
There was a problem hiding this comment.
ops-codeowners: these lines are removed because the tests are all running as part of the same invocation again, so we're not losing any tests, we're just running them the standard way again (ie. removing a special case no longer needed)
|
/merge |
1 similar comment
|
/merge |
…4092) Hooray for removing and cleaning code! Tests also added (we already tested isolated nodes for Louvain). nx-cugraph was updated to handle isolated nodes by passing the node set to PLC in #4077 Authors: - Erik Welch (https://github.com/eriknw) Approvers: - Rick Ratzel (https://github.com/rlratzel) URL: #4092
nx_cugraph.Graphclass to create PLC graphs usingvertices_arrayin order to include isolated vertices. This is now needed since the error check added in this PR prevents NetworkX tests from passing if isolated vertices are treated as invalid, so this change prevents that.Closes #4067