Skip to content

BUG: Remove std distribution from ConnectedComponentImageFilter tests + new baselines#6147

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:pr-5836-baselines
Apr 28, 2026
Merged

BUG: Remove std distribution from ConnectedComponentImageFilter tests + new baselines#6147
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:pr-5836-baselines

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Apr 27, 2026

Successor to #5836 (which is blocked on maintainerCanModify=off so the merged baselines could not be wired up). Carries @N-Dekker's RNG fix (preserving authorship), with the WIP: prefix dropped now that the baselines are landed; plus a follow-up commit that points the .cid content links at the new baselines just merged via ITKTestingData#38.

Verified locally on macOS 15 / Apple Clang Release: itkConnectedComponentImageFilterTest{,RGB,2,3} all pass against the new primary baselines, no fall-through to alternates.

Commits
  1. BUG: Remove std distribution from ConnectedComponentImageFilter tests@N-Dekker's commit, authorship preserved; rebased onto current upstream/main and reworded only to drop the WIP: prefix that ghostflow rejected. Replaces std::uniform_int_distribution with an explicit modulo expression so std::mt19937 output reduces identically across libstdc++ / libc++ / MSVC.
  2. BUG: Update ConnectedComponentImageFilter test baselines for deterministic RNG — updates the three primary .cid files to the new merged CIDs and deletes the three .1 per-platform alternates that existed only to paper over the prior cross-stdlib divergence.
New CIDs
File CID Size
Test.png (also reused by ...TestRGB) bafkreibyjpytxv46k6mkxo3sadppjvs6b5vjqwqcz6u4vkzpp2btmge4fm 6655 B
Test2.png bafkreif6vtfmpapapqk5q5xlitj7g72pdihcrrncdf5cwgtnexu36cj6rm 5182 B
Test3.png bafkreibxbxqmcg73nig3ncv7gt2rqdcj7nfw72r3be4r6l5llmqw4wpwru 3015 B

Blobs are in ITKTestingData@gh-pages, commit d89fd19. The data.kitware.com and itk.org/files/ExternalData mirrors may take a few minutes to publish; CDash CI will fall back to IPFS gateways or directly to gh-pages until then.

Diff stat
 .../itkConnectedComponentImageFilterTest.cxx                  | 11 +++++------
 .../itkConnectedComponentImageFilterTestRGB.cxx               | 11 +++++------
 .../BasicFilters/ConnectedComponentImageFilterTest.1.png.cid  |  1 -
 .../BasicFilters/ConnectedComponentImageFilterTest.png.cid    |  2 +-
 .../BasicFilters/ConnectedComponentImageFilterTest2.1.png.cid |  1 -
 .../BasicFilters/ConnectedComponentImageFilterTest2.png.cid   |  2 +-
 .../BasicFilters/ConnectedComponentImageFilterTest3.1.png.cid |  1 -
 .../BasicFilters/ConnectedComponentImageFilterTest3.png.cid   |  2 +-

Closes #5836 once merged.

@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Segmentation Issues affecting the Segmentation module type:Data Changes to testing data labels Apr 27, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review April 27, 2026 11:27
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 27, 2026

Greptile Summary

This PR fixes non-deterministic test output in ConnectedComponentImageFilter by replacing std::uniform_int_distribution (whose reduction step differs across libstdc++/libc++/MSVC) with an explicit modulo expression on the raw mt19937 output, then updates the three primary baseline CIDs to match and removes the three per-platform .1 alternates that existed only to paper over the prior divergence.

Confidence Score: 4/5

Safe to merge; only P2 findings with no functional or correctness risk.

All findings are P2 style/best-practice issues. The core fix (deterministic RNG) is sound and the baseline updates are consistent with the companion data PR. No P0/P1 issues found.

Both .cxx test files need #include <cstdint> for the newly introduced uint32_t usage.

Important Files Changed

Filename Overview
Modules/Segmentation/ConnectedComponents/test/itkConnectedComponentImageFilterTest.cxx Replaces std::uniform_int_distribution with explicit modulo for cross-platform determinism; introduces uint32_t without <cstdint>.
Modules/Segmentation/ConnectedComponents/test/itkConnectedComponentImageFilterTestRGB.cxx Identical RNG fix as the non-RGB test; same missing <cstdint> issue for uint32_t.
Testing/Data/Baseline/BasicFilters/ConnectedComponentImageFilterTest.png.cid CID updated to new deterministic baseline; companion .1 alternate deleted as expected.
Testing/Data/Baseline/BasicFilters/ConnectedComponentImageFilterTest2.png.cid CID updated to new deterministic baseline; companion .1 alternate deleted as expected.
Testing/Data/Baseline/BasicFilters/ConnectedComponentImageFilterTest3.png.cid CID updated to new deterministic baseline; companion .1 alternate deleted as expected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[mt19937 engine\nseeded with 1031571] --> B[randomNumberEngine]
    B --> C["raw value = randomNumberEngine()"]
    C --> D["% (maxValue - minValue + 1) + minValue\n= % 171 + 85"]
    D --> E["static_cast to RGBComponentType\nrange: [85, 255]"]
    E --> F[colormap entry R/G/B channel]
    F --> G[repeat for all 3 channels]
    G --> H[std::generate fills px]
    H --> I[colormap stored]
    I --> J[colormap indexed by label\nto produce output RGB image]
    J --> K[writer produces deterministic PNG\nacross libstdc++ / libc++ / MSVC]
Loading

Reviews (1): Last reviewed commit: "BUG: Update ConnectedComponentImageFilte..." | Re-trigger Greptile

@N-Dekker
Copy link
Copy Markdown
Contributor

Thanks @hjmjohnson !

#5836 (which is blocked on maintainerCanModify=off

Of course, you could have asked me to switch on maintainerCanModify 🤷 By default, I switch off "Allow edits by maintainers", because I want to have a little bit of control over my own PRs. But in this case, I would certainly be willing to allow edits by maintainers.

I'm glad that test data uploading is finally working again! 🎉

I would suggest squashing the two commits into one. Because the first commit cannot be applied "alone". It must go "hand in hand" with the new test data references.

`std::uniform_int_distribution` appears to produce different distributions on
different platforms. For the ConnectedComponentImageFilter tests, it appears
preferable to use the same sequence of random numbers on all platforms.

Follow-up to pull request InsightSoftwareConsortium#5725
commit 415d2ed, "ENH: Replace "vnl_sample.h"
with `<random>` in ConnectedComponent tests", Jan 9, 2026.
@hjmjohnson hjmjohnson merged commit bf222b7 into InsightSoftwareConsortium:main Apr 28, 2026
10 of 16 checks passed
@hjmjohnson hjmjohnson added this to the ITK 6.0 Release Candidate 1 milestone May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Segmentation Issues affecting the Segmentation module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Data Changes to testing data 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.

3 participants