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

[N-Dekker]

Added ImageFileWriter<> specialization that has a static WriteImage(image, fileName) member function. Introduced in various tests in "Modules/Core/Common/test"

[hjmjohnson]

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

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

[N-Dekker]

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

[hjmjohnson]

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.

[N-Dekker]

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.

[hjmjohnson]

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.

[N-Dekker]

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?

[blowekamp]

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.

[dzenanz]

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.

[N-Dekker]

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

[dzenanz]

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

[dzenanz]

I prefer to have two overloads, one for raw pointer one for smart pointer. That way we can just provide image variable name at the point of invocation, see dzenanz/ITKMorphologicalContourInterpolation@a9ac128#diff-2c8d1766ca920beada092bae39ef4684594b715a9eb835bcc35ad15998b2c86aR1339.

Note that itk::Image<bool, dim> needs a distinct overload with my approach. As that is not commonly used, I am not sure whether we want to support it here.

[dzenanz]

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

[N-Dekker]

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 *.

[dzenanz]

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

[N-Dekker]

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

[dzenanz]

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

[dzenanz]

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.

[thewtex]

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.

[dzenanz]

Excellent idea Niels! After this is in, I won't have to make this convenience function in almost each project I write!

[N-Dekker]

Excellent idea Niels! After this is in, I won't have to make this convenience function in almost each project I write!

Thanks @dzenanz. Indeed it wouldn't be the first effort to get a simple WriteImage convenience function in ITK. I found three previous ones:

https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.1/Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterTest.cxx#L67

https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.1/Modules/IO/Meta/test/itkMetaImageIOMetaDataTest.cxx#L63

https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.1/Modules/IO/ImageBase/include/itkIOTestHelper.h#L78

[dzenanz]

Should we add a convenience TImage::Pointer itk::ReadImage<TImage>(filename)? I frequently make this too.

[N-Dekker]

@dzenanz

Should we add a convenience TImage::Pointer itk::ReadImage<TImage>(filename)? I frequently make this too.

Sounds very useful. Moreover, I would very much like to have a convenience itk::WriteTransform(transform, filename) function.

[dzenanz]

Maybe we should have this for v5.2.0?

There are already similar convenience functions in Python. Would the new C++ functions interfere with that?

[thewtex]

There are already similar convenience functions in Python. Would the new C++ functions interfere with that?

I don't think they would interfere. But, we should use the raw pointer in the interface to avoid wrapping issues.

[N-Dekker]

Please feel free to make a new pull request that has an interface that you prefer.

[dzenanz]

My preferred API is in #2160. Please review.

[N-Dekker]

My preferred API is in #2160. Please review.

Thanks @dzenanz But you merged #2160 already! Do you still want it to be reviewed?

[dzenanz]

Hans merged it. Yes, another review is still welcome. I could make changes in a new PR.

[dzenanz]

This PR is transmuted into #2160 and #2165.