Changed seeding strategy and have one generator per row of output data #2297
Conversation
…per row of output data.
| std::string label, size_t object_width, | ||
| size_t object_height, long seed) { | ||
| parameter_sampler.sample(seed); | ||
| std::string label, |
There was a problem hiding this comment.
Typically this should either be just std::string if we want to pass by value and const std::string& if we want to pass by reference. See https://google.github.io/styleguide/cppguide.html#Use_of_const
For a function parameter passed by value, const has no effect on the caller, thus is not recommended in function declarations.
Here I would recommend a const reference, since we don't modify the value
There was a problem hiding this comment.
Agreed. It should be a const reference.
| // all starter images and the respective chunk of background images | ||
| const flex_image &object = row[image_column_index].get<flex_image>(); | ||
| std::string label = row[target_column_index].to<flex_string>(); | ||
| for (const auto &row : decompressed_data.range_iterator()) { |
There was a problem hiding this comment.
Can you avoid using auto here and on line 225?
There was a problem hiding this comment.
FYI: https://google.github.io/styleguide/cppguide.html#Type_deduction
The fundamental rule is: use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type.
| std::string label, size_t object_width, | ||
| size_t object_height, long seed) { | ||
| parameter_sampler.sample(seed); | ||
| std::string label, |
There was a problem hiding this comment.
Typically this should either be just std::string if we want to pass by value and const std::string& if we want to pass by reference. See https://google.github.io/styleguide/cppguide.html#Use_of_const
For a function parameter passed by value, const has no effect on the caller, thus is not recommended in function declarations.
Here I would recommend a const reference, since we don't modify the value
| @@ -149,14 +150,14 @@ create_synthetic_image_from_background_and_starter(const flex_image &starter, | |||
| reinterpret_cast<const boost::gil::rgba8_pixel_t *>( | |||
| rgba_flex_image.get_image_data()), | |||
| rgba_flex_image.m_channels * | |||
| rgba_flex_image.m_width // row length in bytes | |||
| rgba_flex_image.m_width // row length in bytes | |||
There was a problem hiding this comment.
Any reason to remove the space here? I recommend we just use clang-format to resolve formatting decisions and be done with it.
In this case, I believe clang-format should leave two spaces: https://google.github.io/styleguide/cppguide.html#Implementation_Comments
| // all starter images and the respective chunk of background images | ||
| const flex_image &object = row[image_column_index].get<flex_image>(); | ||
| std::string label = row[target_column_index].to<flex_string>(); | ||
| for (const auto &row : decompressed_data.range_iterator()) { |
There was a problem hiding this comment.
FYI: https://google.github.io/styleguide/cppguide.html#Type_deduction
The fundamental rule is: use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type.
| std::uniform_int_distribution<int> index_distribution(0, INT_MAX); | ||
| theta_mean = theta_means_[index_distribution(engine) % theta_means_.size()]; | ||
| phi_mean = phi_means_[index_distribution(engine) % phi_means_.size()]; | ||
| gamma_mean = gamma_means_[index_distribution(engine) % gamma_means_.size()]; |
There was a problem hiding this comment.
It seems strange to take the result modulo the number of means. Why not just generate a random integer in the desired range right off the bat? Could be a useful helper function here: given a non-inclusive upper bound on the range of the random variable, and the random engine, return a random index
| std::normal_distribution<double> theta_distribution(theta_mean, angle_stdev_); | ||
| std::normal_distribution<double> phi_distribution(phi_mean, angle_stdev_); | ||
| std::normal_distribution<double> gamma_distribution(gamma_mean, angle_stdev_); | ||
| std::normal_distribution<double> focal_distribution((double)width_, | ||
| std::normal_distribution<double> focal_distribution((double)background_width, |
There was a problem hiding this comment.
Let's use static_cast here.
| @@ -62,6 +63,12 @@ void ParameterSampler::set_warped_corners( | |||
| warped_corners_[3] = warped_corners[2]; | |||
| } | |||
|
|
|||
| int generate_random_index(std::mt19937 engine, int range) { | |||
There was a problem hiding this comment.
Do not copy the engine, otherwise you're actually going to get the same random values every time. Here you must pass the engine by reference or as a pointer. Your goal is to reuse the same engine across all the random draws for a single row, not produce draws from individual copies of the original engine.
No description provided.