Skip to content

Replace std::random_shuffle with std::shuffle in tests for objdetect#22228

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
CSharperMantle:CSharperMantle-patch-std-shuffle
Jul 13, 2022
Merged

Replace std::random_shuffle with std::shuffle in tests for objdetect#22228
opencv-pushbot merged 1 commit intoopencv:3.4from
CSharperMantle:CSharperMantle-patch-std-shuffle

Conversation

@CSharperMantle
Copy link
Copy Markdown
Contributor

@CSharperMantle CSharperMantle commented Jul 11, 2022

std::random_shuffle with two arguments is deprecated since C++11 and removed since C++17. This PR adds conditional compilation directives to replace it with the recommended std::shuffle (together with manually-constructed RNGs) when C++11 is available in the test files for objdetect module.

Resolves #22209.

Pull Request Readiness Checklist

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

// Use manually constructed RNG and std::shuffle instead.
std::random_device rand_dev;
// Use Mersenne Twister RNG.
std::mt19937 rand_gen {rand_dev()};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OpenCV tests should be reproducible (otherwise we would have random failures which no-one could debug).
So we should not use "true" random input.
We should use pseudo-random generators with reproducible results.

Initial code was incorrect too.
Prefer to reuse cv::randShuffle() over CV_8U data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is manually seeding the PRNG to a fixed initial state acceptable? Shuffling std::string with cv::randShuffle seems to require more obscure code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, fixed seed should work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Also manually seeded and tested legacy call to std::random_shuffle (although I don't know if it is the most suitable way to do so).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor mistakes corrected in code; it should work now.

@CSharperMantle CSharperMantle force-pushed the CSharperMantle-patch-std-shuffle branch 2 times, most recently from 2ec5a41 to 991d116 Compare July 12, 2022 12:04
- Add conditional compilation directives to replace deprecated std::random_shuffle with new std::shuffle when C++11 is available.

- Set random seed to a fixed value before shuffling containers to ensure reproducibility.

Resolves opencv#22209.
@CSharperMantle CSharperMantle force-pushed the CSharperMantle-patch-std-shuffle branch from 991d116 to 3135063 Compare July 12, 2022 12:19
@CSharperMantle
Copy link
Copy Markdown
Contributor Author

@alalek, would you mind re-approving the Actions workflows? The previous run failed due to some typos in my code, which have been fixed already in the latest commit. Thanks!

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Well done! Thank you 👍

@opencv-pushbot opencv-pushbot merged commit afe1c70 into opencv:3.4 Jul 13, 2022
@CSharperMantle
Copy link
Copy Markdown
Contributor Author

CSharperMantle commented Jul 13, 2022

Thank you for merging this! Could you please close #22209 as well?

@alalek alalek mentioned this pull request Jul 26, 2022
This was referenced Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants