Skip to content

ENH: Replace "vnl/vnl_sample.h" with <random>, in Core tests - #5611

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:Replace-vnl_sample_uniform
Nov 8, 2025
Merged

ENH: Replace "vnl/vnl_sample.h" with <random>, in Core tests#5611
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:Replace-vnl_sample_uniform

Conversation

@N-Dekker

@N-Dekker N-Dekker commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

Replaced code like randomNumber = vnl_sample_uniform(min, max) with something like:

std::mt19937                 randomNumberEngine{};
std::uniform_distribution<T> randomNumberDistribution(min, max);

randomNumber = randomNumberDistribution(randomNumberEngine);

vnl_sample_uniform is discouraged, especially because it has a global state (static data). This problem is addressed by declaring random number engines as local (non-static) variables, local to the tests that use them.


Obviously, this modernization yields more lines of code than before. While in general, I like it better when modernization yields less code! But I can't help it 🤷 It appears fundamental to modern random number generation to declare separate "engine" and "distribution" variables, instead of just doing a single library function call.

@github-actions github-actions Bot added type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Nov 8, 2025
Replaced code like `randomNumber = vnl_sample_uniform(min, max)` with something like:

    std::mt19937                 randomNumberEngine{};
    std::uniform_distribution<T> randomNumberDistribution(min, max);

    randomNumber = randomNumberDistribution(randomNumberEngine);

`vnl_sample_uniform` is discouraged, especially because it has a global state
(static data). This problem is addressed by declaring random number engines
as local (non-static) variables, local to the tests that use them.
@N-Dekker
N-Dekker force-pushed the Replace-vnl_sample_uniform branch from aa8441c to 819856d Compare November 8, 2025 12:09
@N-Dekker
N-Dekker marked this pull request as ready for review November 8, 2025 17:21

@hjmjohnson hjmjohnson left a comment

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.

Lovely!!!

@N-Dekker

N-Dekker commented Nov 8, 2025

Copy link
Copy Markdown
Contributor Author

Lovely!!!

Thanks Hans! There are a few more vnl_sample_uniform calls in tests outside of the Core, but I would rather have them replaced in a separate follow-up pull request, once the current one is fully processed 😃

@hjmjohnson
hjmjohnson merged commit f7daf24 into InsightSoftwareConsortium:main Nov 8, 2025
17 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 10, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 10, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 10, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 10, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit that referenced this pull request Nov 11, 2025
Follow-up to pull request #5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit that referenced this pull request Nov 11, 2025
Follow-up to pull request #5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit that referenced this pull request Nov 11, 2025
Follow-up to pull request #5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit that referenced this pull request Nov 11, 2025
Follow-up to pull request #5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 11, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 11, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 11, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit that referenced this pull request Nov 12, 2025
Follow-up to pull request #5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 12, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 12, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 12, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
CavRiley pushed a commit to CavRiley/ITK that referenced this pull request Dec 23, 2025
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
CavRiley pushed a commit to CavRiley/ITK that referenced this pull request Jan 8, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit f7daf24
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"

The random number generator change affected test outputs, requiring updated baseline images as well.

Co-authored-by: Niels Dekker <N.Dekker@lumc.nl>
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 0f1dd41
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"

The random number generator change affected test outputs, requiring updated baseline images as well.

Co-authored-by: Niels Dekker <N.Dekker@lumc.nl>
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 12, 2026
Follow-up to pull request InsightSoftwareConsortium#5611
commit 2efcc80
"ENH: Replace "vnl/vnl_sample.h" with `<random>`, in Core tests"

The random number generator change affected test outputs, requiring updated baseline images as well.

Co-authored-by: Niels Dekker <N.Dekker@lumc.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants