-
-
Notifications
You must be signed in to change notification settings - Fork 713
WIP: Replace "vnl_sample.h" with <random> in ConnectedComponent tests
#5622
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.
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
base: main
Are you sure you want to change the base?
WIP: Replace "vnl_sample.h" with <random> in ConnectedComponent tests
#5622
Conversation
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on a glance.
|
18ae855 to
3324070
Compare
<random> in Segmentation tests<random> in ConnectedComponent tests
|
There appeared test failures at https://open.cdash.org/viewTest.php?onlyfailed&buildid=10817138 on the following four tests:
I'm sorry, I don't really know how to fix them, but I guess we would have to change the ground truth images (somewhere in ITK/Modules/Segmentation/ConnectedComponents/test/CMakeLists.txt Lines 44 to 55 in 6568876
Does anyone have suggestion? |
Modules/Segmentation/ConnectedComponents/test/itkConnectedComponentImageFilterTest.cxx
Show resolved
Hide resolved
| using RGBComponentType = RGBPixelType::ComponentType; | ||
| constexpr auto maxRGBComponentValue = std::numeric_limits<RGBComponentType>::max(); | ||
|
|
||
| std::mt19937 randomNumberEngine{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@N-Dekker does {} initialize the seed to a constant value?
I think
constexpr auto random_seed = 1031571;
std::mt19937 randomNumberEngine{random_seed};would make this easier to interpret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default constructor of std::mt19937 always uses the same seed: default_seed = 5489. That's different from 1031571, obviously. So we'll see if using 1031571 will make the CI happy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great to know, but not obvious. I would have guessed the default was to use an unknown changing seed unless explicitly specified. I do think that setting the seed explicitly (with what ever value you want, even 5489) would be more meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the C++ standard specifies that the default-constructor of std::mt19937 always uses the same seed, default_seed, I think we should be allowed to just trust the C++ standard.
We could make it explicit:
std::mt19937 randomNumberEngine(std::mt19937::default_seed);
And then add to our unit tests:
static_assert(std::mt19937::default_seed == 5489, "Check on any platform!");
But that all seems overdone to me. If we want to have non-deterministic random numbers, we should use std::random_device.
3324070 to
3dec7c3
Compare
Follow-up to pull request InsightSoftwareConsortium#5611 commit f7daf24 "ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
3dec7c3 to
d993427
Compare
<random>, in Core tests #5611