Use GenPC (Permuted Congruential) as the default random number gene…

…rator everywhere (#1099) `GenPC` is already the default generator used by `RngState` when not passed to the constructor: https://github.com/rapidsai/raft/blob/a9e060d354917114bb8baa5de9c55ef917f203af/cpp/include/raft/random/rng_state.hpp#L48-L52 But the default value for the `GeneratorType` remained `GenPhilox` in some prims, such as `make_blobs` and `make_regression`. PC is faster and more reliable, so it should be the default there too. Also, whenever possible, the generator type shouldn't be hardcoded, it should either use the default (construct `RngState` with the seed only) or an argument (as is the case with `make_blobs` and `make_regression`). _Note: this will effectively modify many test inputs, so be aware of that when comparing results prior to and following the change._ Authors: - Louis Sugy (https://github.com/Nyrio) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #1099
rapidsai · Dec 15, 2022 · 489aa9a · 489aa9a
1 parent 81650cd
commit 489aa9a
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 16 deletions.
diff --git a/cpp/include/raft/cluster/kmeans_types.hpp b/cpp/include/raft/cluster/kmeans_types.hpp
@@ -75,8 +75,7 @@ struct KMeansParams {
   /**
    * Seed to the random number generator.
    */
-  raft::random::RngState rng_state =
-    raft::random::RngState(0, raft::random::GeneratorType::GenPhilox);
+  raft::random::RngState rng_state{0};
 
   /**
    * Metric to use for distance computation.

diff --git a/cpp/include/raft/random/detail/make_regression.cuh b/cpp/include/raft/random/detail/make_regression.cuh
@@ -158,7 +158,7 @@ void make_regression_caller(const raft::handle_t& handle,
                             DataT noise                      = (DataT)0.0,
                             bool shuffle                     = true,
                             uint64_t seed                    = 0ULL,
-                            raft::random::GeneratorType type = raft::random::GenPhilox)
+                            raft::random::GeneratorType type = raft::random::GenPC)
 {
   n_informative = std::min(n_informative, n_cols);
 

diff --git a/cpp/include/raft/random/make_blobs.cuh b/cpp/include/raft/random/make_blobs.cuh
@@ -74,7 +74,7 @@ void make_blobs(DataT* out,
                 DataT center_box_min           = (DataT)-10.0,
                 DataT center_box_max           = (DataT)10.0,
                 uint64_t seed                  = 0ULL,
-                GeneratorType type             = GenPhilox)
+                GeneratorType type             = GenPC)
 {
   detail::make_blobs_caller(out,
                             labels,
@@ -140,7 +140,7 @@ void make_blobs(
   DataT center_box_min                                                   = (DataT)-10.0,
   DataT center_box_max                                                   = (DataT)10.0,
   uint64_t seed                                                          = 0ULL,
-  GeneratorType type                                                     = GenPhilox)
+  GeneratorType type                                                     = GenPC)
 {
   if (centers.has_value()) {
     RAFT_EXPECTS(centers.value().extent(0) == (IdxT)n_clusters,

diff --git a/cpp/include/raft/random/make_regression.cuh b/cpp/include/raft/random/make_regression.cuh
@@ -82,7 +82,7 @@ void make_regression(const raft::handle_t& handle,
                      DataT noise         = (DataT)0.0,
                      bool shuffle        = true,
                      uint64_t seed       = 0ULL,
-                     GeneratorType type  = GenPhilox)
+                     GeneratorType type  = GenPC)
 {
   detail::make_regression_caller(handle,
                                  out,
@@ -149,7 +149,7 @@ void make_regression(const raft::handle_t& handle,
                      DataT noise         = DataT{},
                      bool shuffle        = true,
                      uint64_t seed       = 0ULL,
-                     GeneratorType type  = GenPhilox)
+                     GeneratorType type  = GenPC)
 {
   const auto n_samples = out.extent(0);
   assert(values.extent(0) == n_samples);

diff --git a/cpp/test/linalg/reduce_rows_by_key.cu b/cpp/test/linalg/reduce_rows_by_key.cu
@@ -112,7 +112,7 @@ class ReduceRowTest : public ::testing::TestWithParam<ReduceRowsInputs<T>> {
     rmm::device_uvector<T> weight(0, stream);
     if (params.weighted) {
       weight.resize(nobs, stream);
-      raft::random::RngState r(params.seed, raft::random::GeneratorType::GenPhilox);
+      raft::random::RngState r(params.seed);
       uniform(handle, r, weight.data(), nobs, T(1), params.max_weight);
     }
 

diff --git a/cpp/test/random/make_regression.cu b/cpp/test/random/make_regression.cu
@@ -127,6 +127,9 @@ class MakeRegressionTest : public ::testing::TestWithParam<MakeRegressionInputs<
 
 typedef MakeRegressionTest<float> MakeRegressionTestF;
 const std::vector<MakeRegressionInputs<float>> inputsf_t = {
+  {0.01f, 256, 32, 16, 1, -1, 0.f, true, raft::random::GenPC, 1234ULL},
+  {0.01f, 1000, 100, 47, 4, 65, 4.2f, true, raft::random::GenPC, 1234ULL},
+  {0.01f, 20000, 500, 450, 13, -1, -3.f, false, raft::random::GenPC, 1234ULL},
   {0.01f, 256, 32, 16, 1, -1, 0.f, true, raft::random::GenPhilox, 1234ULL},
   {0.01f, 1000, 100, 47, 4, 65, 4.2f, true, raft::random::GenPhilox, 1234ULL},
   {0.01f, 20000, 500, 450, 13, -1, -3.f, false, raft::random::GenPhilox, 1234ULL}};
@@ -147,6 +150,9 @@ INSTANTIATE_TEST_CASE_P(MakeRegressionTests, MakeRegressionTestF, ::testing::Val
 
 typedef MakeRegressionTest<double> MakeRegressionTestD;
 const std::vector<MakeRegressionInputs<double>> inputsd_t = {
+  {0.01, 256, 32, 16, 1, -1, 0.0, true, raft::random::GenPC, 1234ULL},
+  {0.01, 1000, 100, 47, 4, 65, 4.2, true, raft::random::GenPC, 1234ULL},
+  {0.01, 20000, 500, 450, 13, -1, -3.0, false, raft::random::GenPC, 1234ULL},
   {0.01, 256, 32, 16, 1, -1, 0.0, true, raft::random::GenPhilox, 1234ULL},
   {0.01, 1000, 100, 47, 4, 65, 4.2, true, raft::random::GenPhilox, 1234ULL},
   {0.01, 20000, 500, 450, 13, -1, -3.0, false, raft::random::GenPhilox, 1234ULL}};

diff --git a/cpp/test/random/rng_discrete.cu b/cpp/test/random/rng_discrete.cu
@@ -178,16 +178,16 @@ class RngDiscreteTest : public ::testing::TestWithParam<RngDiscreteInputs<IdxT>>
 };
 
 const std::vector<RngDiscreteInputs<int>> inputs_i32 = {
-  {1, 10000, 5, 5, GenPhilox, 123ULL},
-  {1, 10000, 10, 7, GenPhilox, 456ULL},
-  {1000, 100, 10000, 20, GenPhilox, 123ULL},
-  {1, 10000, 5, 5, GenPC, 1234ULL},
+  {1, 10000, 5, 5, GenPC, 123ULL},
+  {1, 10000, 10, 7, GenPC, 456ULL},
+  {1000, 100, 10000, 20, GenPC, 123ULL},
+  {1, 10000, 5, 5, GenPhilox, 1234ULL},
 };
 const std::vector<RngDiscreteInputs<int64_t>> inputs_i64 = {
-  {1, 10000, 5, 5, GenPhilox, 123ULL},
-  {1, 10000, 10, 7, GenPhilox, 456ULL},
-  {1000, 100, 10000, 20, GenPhilox, 123ULL},
-  {1, 10000, 5, 5, GenPC, 1234ULL},
+  {1, 10000, 5, 5, GenPC, 123ULL},
+  {1, 10000, 10, 7, GenPC, 456ULL},
+  {1000, 100, 10000, 20, GenPC, 123ULL},
+  {1, 10000, 5, 5, GenPhilox, 1234ULL},
 };
 
 #define RNG_DISCRETE_TEST(test_type, test_name, test_inputs)       \