Skip to content

ENH: Add static ImageFileWriter<>::WriteImage(image, fileName) member function #2102

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.

Sign up for GitHub

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

Closed
N-Dekker wants to merge 2 commits into from
Closed

ENH: Add static ImageFileWriter<>::WriteImage(image, fileName) member function #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a8e5d3
ENH: Add static ImageFileWriter<>::WriteImage(image, fileName) function
N-Dekker Nov 6, 2020
7c0cd30
STYLE: Use the static ImageFileWriter<>::WriteImage in Core/Common/test
N-Dekker Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions Modules/Core/Common/test/itkImageDuplicatorTest2.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ itkImageDuplicatorTest2(int argc, char * argv[])

using ReaderType = itk::ImageFileReader<ImageType>;
ReaderType::Pointer reader = ReaderType::New();
using WriterType = itk::ImageFileWriter<ImageType>;
WriterType::Pointer writer = WriterType::New();
using DuplicatorType = itk::ImageDuplicator<ImageType>;
DuplicatorType::Pointer dup = DuplicatorType::New();
using AbsType = itk::AbsImageFilter<ImageType, ImageType>;
Expand Down Expand Up @@ -69,9 +67,7 @@ itkImageDuplicatorTest2(int argc, char * argv[])
dup->Update();
ImageType::ConstPointer dupImage = dup->GetOutput();

writer->SetInput(dupImage);
writer->SetFileName(argv[2]);
writer->Update();
itk::ImageFileWriter<>::WriteImage(*dupImage, argv[2]);
Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

I don't like having to write *varName, I prefer just varName. See declaration suggestion on how to accomplish this.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

My proposal was to use an image reference, rather than a pointer, because a pointer in C++ is used to indicate "might be null". So I wonder, why would someone want to call WriteImage with a (possible) null pointer as image?

A smart pointer is specifically used to indicate that the ownership on an object is shared between entities. It does make sense to share with an ("old style") ImageFileWriter<ImageType> object, but I wonder if it makes sense to share with a static member function call.

That's why I'm reluctant to declare the image parameter as a pointer. But I do see your point that it is cumbersome to always add a *.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

@N-Dekker If a user has pointer which might be null, and wants to print it if it is not null, he will have to write an if. And if we require a reference, he will have to write an if before converting it to a reference. As all ITK images are held through pointers or smart pointers, accepting those is more natural than expecting a reference. I don't see much practical difference between the two, except that we will always have to add * during invocation with the reference approach.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

@dzenanz Just a few links to the C++ Core Guidelines to clarify my reluctance to declare the image as a pointer (smart or raw pointer):

"Take smart pointers as parameters only to explicitly express lifetime semantics"
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rr-smartptrparam

"For general use, take T* or T& arguments rather than smart pointers"
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#f7-for-general-use-take-t-or-t-arguments-rather-than-smart-pointers

"For “in” parameters, pass cheaply-copied types by value and others by reference to const"
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#f7-for-general-use-take-t-or-t-arguments-rather-than-smart-pointers

"Prefer T* over T& when “no argument” is a valid option"
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#f60-prefer-t-over-t-when-no-argument-is-a-valid-option

Now I know that there are a lot of old ITK (member) functions that have pointers as argument types. But for a newly added function maybe we could move forward to use a reference as argument type instead...

Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

We would be explicitly taking itk::SmartPointer<Image<...>> in order to aid template parameter deduction. And primary specialization would be taking Image *. And if you want to be super-correct, you can check for null to avoid crashing.

As we can only hold a (smart) pointer to an ITK image in a variable, this is what we should be passing around. Entire ITK API takes either Image * or Image::Pointer (aka itk::SmartPointer<Image<...>>). Trying to switch one function to taking an Image & seems unnecessary and would make its use harder (specifically having to dereference image at each place of invocation).

Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

And as you might guess, refactoring ITK to change convention from passing Image* to passing Image& is a huge task which breaks backwards compatibility with little tangible gain.

Copy link
Copy Markdown
Member

@thewtex thewtex Nov 24, 2020

Choose a reason for hiding this comment

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

And if you want to be super-correct, you can check for null to avoid crashing.

This function would likely be used for debugging, so it would be good add a null check and a check that the image has been allocated.

There are other issues with the wrapping to use raw pointers instead of references.

std::cout << "Test SUCCESS" << std::endl;
}
catch (const itk::ExceptionObject & e)
Expand Down
8 changes: 2 additions & 6 deletions Modules/Core/Common/test/itkImageRandomIteratorTest2.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,9 @@ itkImageRandomIteratorTest2(int argc, char * argv[])
using PixelType = unsigned long;

using ImageType = itk::Image<PixelType, ImageDimension>;
using WriterType = itk::ImageFileWriter<ImageType>;

ImageType::Pointer image = ImageType::New();

WriterType::Pointer writer = WriterType::New();

ImageType::SizeType size;

size[0] = 1000;
Expand Down Expand Up @@ -84,9 +81,8 @@ itkImageRandomIteratorTest2(int argc, char * argv[])
++counter;
}

writer->SetInput(image);
writer->SetFileName(argv[1]);
writer->Update();

itk::ImageFileWriter<>::WriteImage(*image, argv[1]);

if (argc > 4)
{
Expand Down
8 changes: 1 addition & 7 deletions Modules/Core/Common/test/itkStreamingImageFilterTest3.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,7 @@ itkStreamingImageFilterTest3(int argc, char * argv[])
streamer->SetNumberOfStreamDivisions(numberOfStreamDivisions);
streamer->SetRegionSplitter(splitter);


using WriterType = itk::ImageFileWriter<ImageType>;
WriterType::Pointer writer = WriterType::New();
writer->SetFileName(outputFilename);
writer->SetInput(streamer->GetOutput());
writer->Update();

itk::ImageFileWriter<>::WriteImage(*(streamer->GetOutput()), outputFilename);

unsigned int expectedNumberOfStreams =
splitter->GetNumberOfSplits(streamer->GetOutput()->GetLargestPossibleRegion(), numberOfStreamDivisions);
Expand Down
9 changes: 1 addition & 8 deletions Modules/Core/Common/test/itkSymmetricSecondRankTensorImageReadTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,9 @@ itkSymmetricSecondRankTensorImageReadTest(int ac, char * av[])
++itr;
}

using MatrixWriterType = itk::ImageFileWriter<MatrixImageType>;

MatrixWriterType::Pointer matrixWriter = MatrixWriterType::New();

matrixWriter->SetInput(matrixImage);
matrixWriter->SetFileName(av[1]);

try
{
matrixWriter->Update();
itk::ImageFileWriter<>::WriteImage(*matrixImage, av[1]);
}
catch (const itk::ExceptionObject & excp)
{
Expand Down
9 changes: 1 addition & 8 deletions Modules/Core/Common/test/itkSymmetricSecondRankTensorImageWriteReadTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,9 @@ itkSymmetricSecondRankTensorImageWriteReadTest(int ac, char * av[])
++itr;
}

using TensorWriterType = itk::ImageFileWriter<TensorImageType>;

TensorWriterType::Pointer tensorWriter = TensorWriterType::New();

tensorWriter->SetInput(tensorImageInput);
tensorWriter->SetFileName(av[1]);

try
{
tensorWriter->Update();
itk::ImageFileWriter<>::WriteImage(*tensorImageInput, av[1]);
}
catch (const itk::ExceptionObject & excp)
{
Expand Down
28 changes: 27 additions & 1 deletion Modules/IO/ImageBase/include/itkImageFileWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class ITKIOImageBase_EXPORT ImageFileWriterException : public ExceptionObject
* \sphinxexample{IO/ImageBase/WriteAnImage,Write An image}
* \endsphinx
*/
template <typename TInputImage>
template <typename TInputImage = void>
class ITK_TEMPLATE_EXPORT ImageFileWriter : public ProcessObject
{
public:
Expand Down Expand Up @@ -235,6 +235,32 @@ class ITK_TEMPLATE_EXPORT ImageFileWriter : public ProcessObject
int m_CompressionLevel{ -1 };
bool m_UseInputMetaDataDictionary{ true };
};

template <>
class ImageFileWriter<void>
{
public:
ITK_DISALLOW_COPY_AND_MOVE(ImageFileWriter);

ImageFileWriter() = delete;
~ImageFileWriter() = delete;

/** Writes an image to the specified file. Example:
\code
itk::ImageFileWriter<>::WriteImage(*imagePointer, outputFileName);
\endcode
*/
template <typename TInputImage>
static void
WriteImage(const TInputImage & image, const std::string & fileName)
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson Nov 6, 2020

Choose a reason for hiding this comment

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

?? Should we expose other conditional behaviors as default options?

UseCompression = false, CompressionLevel = -1, UseINputMetaDataDictionary=true)

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

Good question. The aim of this PR is to make the most common use case a simple "one-liner".

Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson Nov 6, 2020

Choose a reason for hiding this comment

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

My solution does not change the most common use case, but does allow for more flexibility easily for those who need it. The UseCompression is the issue I am most interested in maintaining access to.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

itk::ImageFileWriter<ImageType> is being used by ITK itself more than 500 times. I wonder how many times (%) they just assume the default values for those options.

Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson Nov 6, 2020

Choose a reason for hiding this comment

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

Internal to ITK defaults are used almost 100% of the time.

In my applications, and many of the applications that I work on, defaults are used near 0% of the time because I explicitly want my applications to use compression for cost storage reasons.

I love you code simplification solution, and would implement it's used in my applications as long as I don't lose the ability to compress data.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

Of course, extra options could still be added as function parameters with default argument values. (Possibly afterwards.)

Another redesign could be to add those options as data members to the new itk::ImageFileWriter<void>, very much like the original ImageFileWriter<ImageType>, but then still support a "one-liner" like:

itk::ImageFileWriter<>{}.WriteImage(image, fileName);

Which would default-construct a writer (containing default values for its options), and then use it directly, calling a non-static WriteImage member function that takes the options into account. Or classic ITK New() style:

itk::ImageFileWriter<>::New()->WriteImage(image, fileName);

For the moment I still prefer the current PR (just a static member function), possible with some optional default arguments. What do you think?

Copy link
Copy Markdown
Member

@blowekamp blowekamp Nov 6, 2020

Choose a reason for hiding this comment

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

If the proposed option is implemented UseCompression = false, CompressionLevel = -1, UseINputMetaDataDictionary=true then we will have calls like:

itk::ImageFileWriter<void>::WriteImage(image, "something.ext", true, 9, true);

In C++ the unnamed parameters are not clear what the argument position means.

Perhaps adding a WriteImageWithCompression function would satisfy this added requirements?

I prefer to have two overloads, one for raw pointer one for smart pointer.

There should be implicit conversions from smart pointers to raw pointers, therefor just a method with a raw pointer should work.

This is a new style for ITK, we have not had function like calls before for process object. An alternative for the void template parameters would be to place the template function(s) into a namespace.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

There should be implicit conversions from smart pointers to raw pointers

Maybe, but if there is no exact match template arguments cannot be deduced.

place the template function(s) into a namespace

I like it! So we could have:

#include "ImageFileWriter.h"
...
itk::WriteImage(image, fileName);
itk::WriteImage(filter->GetOutput(), fileName2, true); // compressed

I like default UseCompression = false better than WriteImageWithCompression, but I could live with the latter too.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Nov 6, 2020

Choose a reason for hiding this comment

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

I like the idea of having a WriteImage in itk namespace, rather than as a static member of itk::ImageFileWriter<void>. The only (minor) drawback could be that it might be less obvious where to define itk::WriteImage. Should it then be in its own "itkWriteImage.h"?

Copy link
Copy Markdown
Member

@dzenanz dzenanz Nov 6, 2020

Choose a reason for hiding this comment

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

It would have to include itkImageFileWriter.h anyway, so a separate include file would reduce its convenience somewhat. @blowekamp what is your preference?

{
const auto writer = itk::ImageFileWriter<TInputImage>::New();
writer->SetFileName(fileName);
writer->SetInput(&image);
writer->Update();
}
};

} // end namespace itk

#ifndef ITK_MANUAL_INSTANTIATION
Expand Down