Skip to content

Add and use itk::ImageBase::AllocateInitialized()#4479

Merged
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:AllocateZeroInitializedPixelBuffer
Feb 27, 2024
Merged

Add and use itk::ImageBase::AllocateInitialized()#4479
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:AllocateZeroInitializedPixelBuffer

Conversation

@N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Feb 24, 2024 •
edited
Loading

The existing three possible ways to call Allocate(bool = false) (either by Allocate(true), Allocate(false), or Allocate()) may accidentally be confused, especially by novice ITK users.

The proposed AllocateZeroInitializedPixelBuffer() is intended to reduce the chance of such a confusion, and improve code readability. It is equivalent to Allocate(true).

For the record, "AllocateZeroInitializedPixelBuffer" was renamed to "AllocateInitialized", as suggested at #4479 (comment)

@github-actions github-actions bot added area:Python wrapping Python bindings for a class 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 area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Numerics Issues affecting the Numerics module labels Feb 24, 2024
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Feb 25, 2024

/azp run

issakomi
issakomi reviewed Feb 25, 2024
View reviewed changes
@N-Dekker N-Dekker marked this pull request as ready for review February 25, 2024 13:40
@blowekamp
Copy link
Member

blowekamp commented Feb 26, 2024

I find the member function name over verbose.

@dzenanz
Copy link
Member

dzenanz commented Feb 26, 2024

AllocateInitialized for the name? Docstring should have sufficient explanation about default values.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Feb 26, 2024

AllocateInitialized for the name?

Thanks for the suggestion @dzenanz. However, I would like the name to be self-explanatory. Until now it was a common practice to add a comment to actually explain the meaning of Allocate(true) with each Allocate(true) call. For example:

m_TemporaryPointer->Allocate(true); // initialize buffer to zero

ITK/Modules/Core/Common/include/itkFloodFilledFunctionConditionalConstIterator.hxx

Line 86 in adcc46b

m_TemporaryPointer->Allocate(true); // initialize buffer to zero

So do you (both) think the name AllocateInitialized is sufficiently self-explanatory? So that most people won't feel the need to look up the meaning, or add a comment like "initialize buffer to zero"?

@blowekamp
Copy link
Member

blowekamp commented Feb 26, 2024

So do you (both) think the name AllocateInitialized is sufficiently self-explanatory? So that most people won't feel the need to look up the meaning, or add a comment like "initialize buffer to zero"?

I think the name is sufficient 👍

@N-Dekker
ENH: Add AllocateInitialized() to ImageBase
47fe345 
Equivalent to `Allocate(true)`. Aims to help improve readability of user code.
@N-Dekker
STYLE: Replace Allocate(true) calls with AllocateInitialized()
527843e 
Replaced function calls of the form `image->Allocate(true)` with
`image->AllocateInitialized()`. Removed redundant comments that said something
like `// initialize buffer to zero`.
@N-Dekker N-Dekker force-pushed the AllocateZeroInitializedPixelBuffer branch from 492c355 to 527843e Compare February 26, 2024 17:28
@N-Dekker N-Dekker changed the title Add and use itk::ImageBase::AllocateZeroInitializedPixelBuffer() Add and use itk::ImageBase::AllocateInitialized() Feb 27, 2024
@dzenanz dzenanz merged commit 1b01d5c into InsightSoftwareConsortium:master Feb 27, 2024
@N-Dekker N-Dekker mentioned this pull request Feb 28, 2024
Merged
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Mar 5, 2024
@N-Dekker
STYLE: Replace Allocate(); FillBuffer(0) with AllocateInitialized()
ce12f9a 
Replaced lines of code of the form

    image>Allocate();
    image>FillBuffer(zero);

With `image->AllocateInitialized();`.

Follow-up to pull request InsightSoftwareConsortium#4479
commit 47fe345
"ENH: Add `AllocateInitialized()` to ImageBase"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Mar 5, 2024
@N-Dekker
STYLE: Replace Allocate(); FillBuffer(0) with AllocateInitialized()
08737f4 
Replaced lines of code of the form

    image>Allocate();
    image>FillBuffer(zero);

With `image->AllocateInitialized();`.

Cases found by the regular expression `^(.+->)Allocate.+;\r\n\1FillBuffer`.

Follow-up to pull request InsightSoftwareConsortium#4479
commit 47fe345
"ENH: Add `AllocateInitialized()` to ImageBase"
@N-Dekker N-Dekker mentioned this pull request Mar 5, 2024
Merged
hjmjohnson pushed a commit that referenced this pull request Mar 6, 2024
@N-Dekker @hjmjohnson
STYLE: Replace Allocate(); FillBuffer(0) with AllocateInitialized()
cd49925 
Replaced lines of code of the form

    image>Allocate();
    image>FillBuffer(zero);

With `image->AllocateInitialized();`.

Cases found by the regular expression `^(.+->)Allocate.+;\r\n\1FillBuffer`.

Follow-up to pull request #4479
commit 47fe345
"ENH: Add `AllocateInitialized()` to ImageBase"
@N-Dekker N-Dekker mentioned this pull request Apr 19, 2024
Draft
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 5, 2024
@N-Dekker
STYLE: GTests replace Allocate(true) with ITK 5.4 AllocateInitialized()
9e4fd5c 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 5, 2024
@N-Dekker
STYLE: Example replace Allocate(true) with ITK 5.4 AllocateInitialized()
13131c6 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
@N-Dekker
STYLE: GTests replace Allocate(true) with ITK 5.4 AllocateInitialized()
d58f863 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
@N-Dekker
STYLE: Example replace Allocate(true) with ITK 5.4 AllocateInitialized()
23c7121 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
@N-Dekker
STYLE: GTests replace Allocate(true) with ITK 5.4 AllocateInitialized()
8ec465d 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
@N-Dekker
STYLE: Example replace Allocate(true) with ITK 5.4 AllocateInitialized()
ca36292 
Following ITK pull request InsightSoftwareConsortium/ITK#4479 commit InsightSoftwareConsortium/ITK@47fe345 "ENH: Add `AllocateInitialized()` to ImageBase".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blowekamp blowekamp blowekamp left review comments

@dzenanz dzenanz dzenanz approved these changes

+1 more reviewer

@issakomi issakomi issakomi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Python wrapping Python bindings for a class area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@N-Dekker @blowekamp @dzenanz @issakomi