ENH: Add benchmark for iterator loop comparison #105
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blowekamp
force-pushed
the
add_iterator_benchmark
branch
from
54f1fbf to
01ad87d
Compare
7 tasks
dzenanz
approved these changes
Dec 4, 2025
Member
|
Compiled with Visual Studio 2026, I get this report when running locally with image size 512: |
hjmjohnson
approved these changes
Dec 4, 2025
Member
|
With image size 256: |
Member
Author
|
Here are my results with a 256 sized image: Compiler: Apple clang version 16.0.0 (clang-1600.0.26.6) |
dzenanz
reviewed
Dec 4, 2025
| auto outputIt = outputRange.begin(); | ||
|
|
||
| const unsigned int componentsPerPixel = itk::NumericTraits<OutputPixelType>::GetLength(*outputIt); | ||
| OutputPixelType outputPixel{ *outputIt }; |
Member
There was a problem hiding this comment.
Suggested change
| OutputPixelType outputPixel{ *outputIt }; | |
| OutputPixelType outputPixelRef{ *outputIt }; | |
| OutputPixelType outputPixel = outputPixelRef; |
Does this change solve the bug? I my local debugging, it seems to do so.
Member
Author
There was a problem hiding this comment.
This still has the bug in it. Let me remove that to get valid benchmarks for the range-based-for loop.
Member
Author
There was a problem hiding this comment.
With that fix I get the same performance with or without the range-based-for loop. I have updated my tables.
blowekamp
force-pushed
the
add_iterator_benchmark
branch
from
01ad87d to
7495758
Compare
N-Dekker
reviewed
Dec 5, 2025
N-Dekker
reviewed
Dec 5, 2025
| #include <iomanip> | ||
| #include <fstream> | ||
|
|
||
|
|
Collaborator
There was a problem hiding this comment.
Please place all helper functions (that is basically everything, except for main) in an unnamed namespace.
N-Dekker
reviewed
Dec 5, 2025
Comment on lines
56
to
57
| itk::ImageRegionIteratorWithIndex<TImage> it(image, region); | ||
| for (it.GoToBegin(); !it.IsAtEnd(); ++it) |
Collaborator
There was a problem hiding this comment.
Please use ImageBufferRange here, instead of ImageRegionIteratorWithIndex.
In general, ImageRegionIteratorWithIndex should only be used when you need the (multi-dimensional) index of each pixel. (That is not the case here.) ImageBufferRange is preferred when the entire image buffer is accessed. (That is the case here!)
Added precommit hook to automatically run clang-format.
blowekamp
force-pushed
the
add_iterator_benchmark
branch
from
7495758 to
b214630
Compare
dzenanz
approved these changes
Dec 9, 2025
Collaborator
|
May I conclude that for these cases, |
Member
Author
When not progress need to be reported. The ScanlineIterator only reports progress once per line, I'm not sure there is a similar pattern with the ImageRegionRange iterator. |
Member
Author
|
The failure is due to macOS-13 being retired. |
blowekamp
merged commit 7817cf6
into
InsightSoftwareConsortium:master
8 of 10 checks passed
Collaborator
Thanks @blowekamp I'm glad to hear that ImageRegionRange is the fastest! I was afraid that ScanlineIterator might have been faster 😇
Good question... I guess the following should do it: TotalProgressReporter progress(this, numberOfPixels);
ImageRegionRange imageRegionRange(*image, region);
auto it = imageRegionRange.begin();
for (unsigned int i{}; i < numberOfPixels; ++i)
{
*it = 42; // Or something more useful, of course....
++it;
if ((i % numberOfPixelsPerLine) == 0)
{
progress.Completed(numberOfPixelsPerLine);
}
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.