Skip to content

Erdos-Renyi generator had bad logic in thrust calls #4362

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Erdos-Renyi generator had bad logic in thrust calls #4362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67466ff
thrust sequence wasn't quite right, random_iterator was returning pro…
ChuckHastings Apr 19, 2024
8514937
remove debug prints, add back in larger tests
ChuckHastings Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 25 additions & 33 deletions cpp/src/generators/erdos_renyi_generator.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@
#include <thrust/copy.h>
#include <thrust/count.h>
#include <thrust/iterator/counting_iterator.h>
#include <thrust/iterator/transform_iterator.h>
#include <thrust/iterator/transform_output_iterator.h>
#include <thrust/iterator/zip_iterator.h>
#include <thrust/random.h>
#include <thrust/transform.h>
#include <thrust/tuple.h>

namespace cugraph {
Expand All @@ -42,45 +41,38 @@ generate_erdos_renyi_graph_edgelist_gnp(raft::handle_t const& handle,
CUGRAPH_EXPECTS(num_vertices < std::numeric_limits<int32_t>::max(),
"Implementation cannot support specified value");

auto random_iterator = thrust::make_transform_iterator(
thrust::make_counting_iterator<size_t>(0),
cuda::proclaim_return_type<float>([seed] __device__(size_t index) {
thrust::default_random_engine rng(seed);
thrust::uniform_real_distribution<float> dist(0.0, 1.0);
rng.discard(index);
return dist(rng);
}));
size_t max_num_edges = static_cast<size_t>(num_vertices) * num_vertices;

size_t count = thrust::count_if(handle.get_thrust_policy(),
random_iterator,
random_iterator + num_vertices * num_vertices,
[p] __device__(float prob) { return prob < p; });

rmm::device_uvector<size_t> indices_v(count, handle.get_stream());
auto generate_random_value = cuda::proclaim_return_type<float>([seed] __device__(size_t index) {
thrust::default_random_engine rng(seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be out side the scope of this bug fix PR, but should we better use raft::random::RngState? Using two different random number generation mechanism does not sound ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created an issue to address this.

thrust::uniform_real_distribution<float> dist(0.0, 1.0);
rng.discard(index);
return dist(rng);
});

thrust::copy_if(handle.get_thrust_policy(),
random_iterator,
random_iterator + num_vertices * num_vertices,
indices_v.begin(),
[p] __device__(float prob) { return prob < p; });
size_t count = thrust::count_if(handle.get_thrust_policy(),
thrust::make_counting_iterator<size_t>(0),
thrust::make_counting_iterator<size_t>(max_num_edges),
[generate_random_value, p] __device__(size_t index) {
return generate_random_value(index) < p;
});

rmm::device_uvector<vertex_t> src_v(count, handle.get_stream());
rmm::device_uvector<vertex_t> dst_v(count, handle.get_stream());

thrust::transform(handle.get_thrust_policy(),
indices_v.begin(),
indices_v.end(),
thrust::make_zip_iterator(thrust::make_tuple(src_v.begin(), src_v.end())),
thrust::copy_if(handle.get_thrust_policy(),
thrust::make_counting_iterator<size_t>(0),
thrust::make_counting_iterator<size_t>(max_num_edges),
thrust::make_transform_output_iterator(
thrust::make_zip_iterator(src_v.begin(), dst_v.begin()),
cuda::proclaim_return_type<thrust::tuple<vertex_t, vertex_t>>(
[num_vertices] __device__(size_t index) {
size_t src = index / num_vertices;
size_t dst = index % num_vertices;

return thrust::make_tuple(static_cast<vertex_t>(src),
static_cast<vertex_t>(dst));
}));

handle.sync_stream();
Copy link

Choose a reason for hiding this comment

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

Is it OK to leave out handle.sync_stream() here, as there is one after the kernel call in cpp/tests/generators/erdos_renyi_test.cpp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The sync_stream call here was never necessary. Any asynchronous work that is active on this stream will continue to be active when we leave this function call.

return thrust::make_tuple(static_cast<vertex_t>(index / num_vertices),
static_cast<vertex_t>(index % num_vertices));
})),
[generate_random_value, p] __device__(size_t index) {
return generate_random_value(index) < p;
});

return std::make_tuple(std::move(src_v), std::move(dst_v));
}
Expand Down
1 change: 1 addition & 0 deletions cpp/tests/generators/erdos_renyi_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ void er_test(size_t num_vertices, float p)
TEST_F(GenerateErdosRenyiTest, ERTest)
{
er_test<int32_t>(size_t{10}, float{0.1});
er_test<int32_t>(size_t{10}, float{0.5});
er_test<int32_t>(size_t{20}, float{0.1});
er_test<int32_t>(size_t{50}, float{0.1});
er_test<int32_t>(size_t{10000}, float{0.1});
Expand Down