Skip to content
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

WIP: Add static member, itk::Image::CreateInitialized(const RegionType &) #4599

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Apr 19, 2024

itk::Image::CreateInitialized aims to simplify creating an image with an allocated buffer of zero-initialized pixels.

Replaced more than sixty cases of code like:

    auto image = ImageType::New();
    image->SetRegions(region);
    image->AllocateInitialized();

with the following single statement:

    auto image = ImageType::CreateInitialized(region);

in ITK test files ("itk*Test*.cxx").

@github-actions github-actions bot added 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 Apr 19, 2024
Comment on lines -168 to +172
auto image = ImageType::New();
{
auto image = ImageType::CreateInitialized([&reader] {
ImageType::RegionType region = reader->GetOutput()->GetLargestPossibleRegion();
region.SetIndex(0, 10);
image->SetRegions(region);
image->AllocateInitialized();
}
return region;
}());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this was probably the most interesting use case that I found. In the original code, the variable region was already kept inside a very small scope, between { and }. The proposed pull request places the region variable inside a lambda, in order to keep its scope small.

@blowekamp
Copy link
Member

Are there any similarly named methods to "Obj::CreateInitialized"? What about something like MakeImage? We already have similarly name methods for other classes?

Note: I'm not suggesting preference, but starting a conversion about what may be the best and most consistent interface.

@N-Dekker
Copy link
Contributor Author

Are there any similarly named methods to "Obj::CreateInitialized"? What about something like MakeImage?

Thanks for asking @blowekamp I agree that it's good to discuss the name.

  • Of course, the currently proposed itk::Image::CreateInitialized is very much inspired by the name itk::ImageBase::AllocateInitialized(), which was the outcome of our discussion at Add and use itk::ImageBase::AllocateInitialized() #4479
  • itk::Image::CreateInitialized is very much meant as an alternative to itk::Image::New(). Having it as a static member function of itk::Image makes it easier to migrate code from using ImageType::New to using ImageType::CreateInitialized.
  • Another option could be to overload Image::New(), so that the new static member function would be itk::Image::New(const RegionType& ), with the region as extra argument. But I wasn't sure if overloading Image::New could cause ambiguities or make things unclear 🤷
  • The name "CreateInitialized" eases adding more functions in the future like "CreateUninitialized" (allocating the buffer, but leaving the pixels uninitialized) or "CreateFilled" (doing a FillBuffer), if there is any interest.
  • A more verbose name is also possible, like "CreateAndSetRegionsAndAllocateInitialized". Doesn't sound very appealing, though!
  • If we'd go for a free function in the itk namespace instead, like itk::MakeImage, we would still need to make a choice about its template argument(s). One template argument, itk::MakeImage<TImage> or two, itk::MakeImage<TPixel, NDimension>? Both have their pro's and cons.

What do you think?

@blowekamp
Copy link
Member

Good points. We also need to consider other image type such as VectorImage, and LabelMap and even module image types like RLE image. Perhaps itk::MakeImage<TImage> could trivially be made to work with all of them?

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Apr 19, 2024

Good points. We also need to consider other image type such as VectorImage, and LabelMap and even module image types like RLE image. Perhaps itk::MakeImage<TImage> could trivially be made to work with all of them?

VectorImage would need an extra parameter to specify the vector length. So I think it would need to have its own static member function VectorImage::CreateInitialized(region, vectorLength) (or its own free function, itk::MakeVectorImage<TVectorImage>(region, vectorLength)).

I'm not sure about supporting LabelMap with this pull request . LabelMap seems so completely different from a regular itk::Image. For example, LabelMap::Allocate(bool initialize = false) does not really allocate anything. It just does ClearLabels(). And it does not seem to differentiate initialize = false versus initialize = true. Is it necessary to do labelMap->SetRegions(region), before using a labelMap, like it is for itk::Image? I don't know 🤷

Just like LabelMap, RLEImage also ignores the initialize parameter of its Allocate(bool initialize = false) member function: https://github.com/KitwareMedical/ITKRLEImage/blob/85559c99ece93d695889dbc908c52efbc2bf50a9/include/itkRLEImage.hxx#L44

Aims to simplify creating an image with an allocated buffer of zero-initialized
pixels.
Replaced more than sixty cases of code like:

    auto image = ImageType::New();
    image->SetRegions(region);
    image->AllocateInitialized();

with the following single statement:

    auto image = ImageType::CreateInitialized(region);

in ITK test files ("itk*Test*.cxx").
@github-actions github-actions bot added the area:Python wrapping Python bindings for a class label Apr 19, 2024
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Apr 22, 2024

@blowekamp I guess a static Image::CreateInitialized(const RegionType &) member function is automatically wrapped by SWIG, for each supported pixel type and image dimension. On the other hand I guess a free function template like itk::MakeImage<TImage> would need to be "SWIG-ed" separately for each supported image type. Right?

Just something to be taken into account. I'm not against a free function template like itk::MakeImage<TImage>, but I still prefer the Image::CreateInitialized(const RegionType &) member function that I'm currently proposing 😃

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 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
Development

Successfully merging this pull request may close these issues.

3 participants