-
-
Notifications
You must be signed in to change notification settings - Fork 715
ENH: Update connected component testing baselines #5725
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
ENH: Update connected component testing baselines #5725
Conversation
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.
Thank you for contributing a pull request! 🙏
Welcome to the ITK community! 🤗👋☀️
We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖
This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.
Testing/Data/Baseline/BasicFilters/ConnectedComponentImageFilterTest.1.png.cid
Show resolved
Hide resolved
|
@CavRiley Would it be nice to squash your commit and mine together? I feel that one commit cannot live without the other 😸 If you like that, feel free to squash the two commits, and then please add me as co-author! (At the end of your commit message, |
|
What were the .1 alternate bases lines generated? They have some ### times been different results from different platforms, when the results are not always consistent. |
|
Ah, well noticed Brad. The baselines should be replaced, not added. |
The I generated the primary Let me know if this answered your question! |
It seems reasonable to me to still keep the old baseline as well, so that at least the final ITK 5.x release can still pass the tests. Right? |
|
Yes, that does answer my question Cavan. Niels, we don't need old test baselines in the main branch. During release process, then-current test data is archived. |
hjmjohnson
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.
@CavRiley Nice work!
N-Dekker
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.
Thanks @CavRiley ! Cool!
Approved, but please consider squashing the two commits 😃
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 <[email protected]>
8d457c0 to
415d2ed
Compare
Sorry I just got to this. I hope it looks alright! |
N-Dekker
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.
Thank you for squashing, @CavRiley! Even more approved! 😃
Thank you for making this contribution! Do you have any insight why different results are occuring? Is the same random image being generated on the two platforms? Or is the filter just producing different results? |
Interesting! I would expect the same pseudo-random number sequence on all platforms 🤷 |
I agree with @N-Dekker in that I would expect the same number sequence on each platform, but my best guess is that |
Wow, that's a pity! Thanks @CavRiley! I did not realize before that the result of So the random number engine produces the same sequence on all platforms, but the "distribution" does not! Before our PR, did all platforms share the same baselines? |
No, previously they did not share the same baselines! I only updated the existing baseline files. |
So originally, the baseline files were already platform dependent! Interesting, thanks @CavRiley! Maybe we could introduce our own (ITK specific) platform-independent alternative to For unit tests, the quality of the distribution usually doesn't matter so much, but for testing, reproducibility does matter. |
Description
This PR updates the baseline images for ConnectedComponent tests following the changes in PR #5622, which replaced
vnl_sample.hwith<random>. The switch to the standard library random number generator produced different random sequences, causing the existing baseline images to fail validation.Related Issues/PRs
Follow-up to #5622
PR Checklist
Testing
Baseline images were regenerated by running the ConnectedComponent tests with the updated random number generator. All tests now pass with the new baselines.