ITK icon indicating copy to clipboard operation
ITK copied to clipboard

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

Open N-Dekker opened this issue 1 year ago • 5 comments

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").

N-Dekker avatar Apr 19 '24 14:04 N-Dekker

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.

blowekamp avatar Apr 19 '24 15:04 blowekamp

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 #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?

N-Dekker avatar Apr 19 '24 16:04 N-Dekker

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?

blowekamp avatar Apr 19 '24 16:04 blowekamp

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

N-Dekker avatar Apr 19 '24 17:04 N-Dekker

@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 😃

N-Dekker avatar Apr 22 '24 11:04 N-Dekker