ENH: remove redundant WriteImage overloads by dzenanz · Pull Request #2165 · InsightSoftwareConsortium/ITK

dzenanz · 2020-12-09T16:37:18Z

This is a follow-up to #2160 which was merged a bit too fast.

N-Dekker · 2020-12-09T17:06:12Z

I would rather postpone the introduction of C++17 std::string_view. No need to rush.

N-Dekker · 2020-12-09T17:29:36Z

I would rather postpone the introduction of C++17 std::string_view. No need to rush.

The thing is, I don't have any std::string_view experience, don't know if it is always OK to just replace any const char* and const std::string & parameter by std::string_view. Then I wonder whether the convention is to pass string_view by value, or by const reference. And if it would be of interest, maybe we could implement a itk::string_view class, to be used until ITK is upgraded to C++17... But again, no need to hurry, I think.

dzenanz · 2020-12-09T17:31:59Z

This convenience function is not a big reason to go down that hole. And generally, ITK does not deal with strings much - mostly for file names and metadata dictionary.

dzenanz · 2020-12-09T17:33:09Z

Maybe we need the char * overloads too: Modules/IO/Meta/test/itkMetaImageIOMetaDataTest.cxx:265:43: error: call of overloaded 'WriteImage(itk::Image<unsigned char, 2u>::Pointer&, char*&)' is ambiguous.

dzenanz · 2020-12-09T17:34:49Z

If we do need them, this PR is pointless. Does anyone have a suggestion of doing this with less than 8 overloads?

blowekamp · 2020-12-09T17:37:43Z

If we do need them, this PR is pointless. Does anyone have a suggestion of doing this with less than 8 overloads?

My initial expectation is that only WriteImage(const TImage * image, const std::string & filename, bool compress = false) should be needed. By not having to resolve which function to use implicit conversions should more readily occur with the compiler.

dzenanz · 2020-12-09T17:46:33Z

@blowekamp 1 signature produces:

10>itkImageDuplicatorTest2.cxx
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageDuplicatorTest2.cxx(70): error C2672: 'itk::WriteImage': no matching overloaded function found
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageDuplicatorTest2.cxx(70): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<const itk::Image<PixelType,3>>'
10>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
10>itkImageRandomIteratorTest2.cxx
11>ITKCommon2TestDriver.cxx
11>itkSymmetricSecondRankTensorTest.cxx
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2672: 'itk::WriteImage': no matching overloaded function found
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<PixelType,2>>'
10>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
10>itkStreamingImageFilterTest3.cxx
12>ITKCommonGTestDriver.vcxproj -> C:\Dev\ITK-git-2015\bin\RelWithDebInfo\ITKCommonGTestDriver.exe
12>ITKCommonGTestDriver.vcxproj -> C:/Dev/ITK-git-2015/bin/RelWithDebInfo/ITKCommonGTestDriver.pdb (Full PDB)
11>Generating Code...
10>itkSymmetricSecondRankTensorImageReadTest.cxx
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2672: 'itk::WriteImage': no matching overloaded function found
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<MatrixPixelType,3>>'
10>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
10>itkSymmetricSecondRankTensorImageWriteReadTest.cxx
11>ITKCommon2TestDriver.vcxproj -> C:\Dev\ITK-git-2015\bin\RelWithDebInfo\ITKCommon2TestDriver.exe
11>ITKCommon2TestDriver.vcxproj -> C:/Dev/ITK-git-2015/bin/RelWithDebInfo/ITKCommon2TestDriver.pdb (Full PDB)
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2672: 'itk::WriteImage': no matching overloaded function found
10>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<TensorPixelType,2>>'
10>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'

If I also add WriteImage(const SmartPointer<const TImage> image, const std::string & filename, bool compress = false):

4>itkImageRandomIteratorTest2.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<PixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(253): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<PixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
4>itkSymmetricSecondRankTensorImageReadTest.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<MatrixPixelType,3>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(253): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<MatrixPixelType,3>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
4>itkSymmetricSecondRankTensorImageWriteReadTest.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<TensorPixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(253): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<TensorPixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'

dzenanz · 2020-12-09T17:48:35Z

I will leave this open for a while, in case someone gets some brilliant ideas. But I am essentially abandoning this PR, and when I see this after a week has passed I will formally close it (assuming no new ideas).

N-Dekker · 2020-12-09T19:03:46Z

Honestly I'm still in favor of passing the image by reference, instead of by smart/raw pointer. None of the overloads would have been necessary then.

dzenanz · 2020-12-09T20:01:43Z

That just places the burden on the user. It becomes annoying really quickly.

N-Dekker · 2020-12-09T20:16:40Z

That just places the burden on the user. It becomes annoying really quickly.

Then why, in your opinion, did C++ introduce pass by (const) reference for parameters?

N-Dekker · 2020-12-09T20:17:49Z

Sorry for accidentally editing your comment @dzenanz !

dzenanz · 2020-12-09T20:26:57Z

ITK was designed in the C++98 world, before shared_ptr was part of standard library. So our syntax does not mesh perfectly with C++11. ITK uses SmartPointers for images exclusively, and trying to work around just makes your life harder.

dzenanz · 2020-12-10T12:20:09Z

const auto writer = ImageFileWriter<TImage>::New(); results in:

4>itkImageRandomIteratorTest2.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<PixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(252): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkImageRandomIteratorTest2.cxx(85): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<PixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
4>itkSymmetricSecondRankTensorImageReadTest.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<MatrixPixelType,3>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(252): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageReadTest.cxx(87): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<MatrixPixelType,3>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'
4>itkSymmetricSecondRankTensorImageWriteReadTest.cxx
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2672: 'itk::WriteImage': no matching overloaded function found
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2784: 'void itk::WriteImage(const itk::SmartPointer<const TImage>,const std::string &,bool)': could not deduce template argument for 'const itk::SmartPointer<const TImage>' from 'itk::SmartPointer<itk::Image<TensorPixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(252): note: see declaration of 'itk::WriteImage'
4>C:\Dev\ITK-git\Modules\Core\Common\test\itkSymmetricSecondRankTensorImageWriteReadTest.cxx(72): error C2784: 'void itk::WriteImage(const TImage *,const std::string &,bool)': could not deduce template argument for 'const TImage *' from 'itk::SmartPointer<itk::Image<TensorPixelType,2>>'
4>c:\dev\itk-git\modules\io\imagebase\include\itkImageFileWriter.h(242): note: see declaration of 'itk::WriteImage'

blowekamp · 2020-12-10T13:44:28Z

I believe some additional conversion with the std::SmartPoitner with const/non-const should help give the convenience of implicit conversion here. Alternatively, we could just a .GetPointer() to the call the WriteImage.

In the convenience vs verbosity, the traditionally chooses to be clear and verbose.

dzenanz · 2020-12-10T13:50:00Z

Most images we want saved are held in a smart pointer, I believe that signature is more important. Also, since smart pointer has a constructor from raw pointer, there should be an automatic conversion. Sadly, compilers fail to do it.

blowekamp · 2020-12-10T14:38:14Z

Most images we want saved are held in a smart pointer, I believe that signature is more important.

I believe you may mean the usage is important, not the signature.

This one function seems to work:

ITK_TEMPLATE_EXPORT void
WriteImage(TImage image, const std::string &filename, bool compress = false)
{
  static_assert(std::is_pointer<TImage>::value || mpl::IsSmartPointer<TImage>::Value,
                "WriteImage requires a raw pointer or SmartPointer.");
  using  ImagePointerType = typename std::conditional< std::is_pointer<TImage>::value,
    SmartPointer<const typename std::remove_pointer<TImage>::type>,
    const TImage>::type;

  using ImageType = typename std::remove_const<typename ImagePointerType::ObjectType>::type;

  ImagePointerType ptr = image;

  auto writer = ImageFileWriter<ImageType>::New();
  writer->SetInput(ptr);
  writer->SetFileName(filename);
  writer->SetUseCompression(compress);
  writer->Update();
}

This might be useful too:


template <typename T>
struct IsSmartPointer: FalseType
{};


template <typename T>
struct IsSmartPointer<SmartPointer<T>>: TrueType
{};

The in function meta-programming could be refactored into a meta-struct, something called like "MakeSmartPointer".

@N-Dekker I bet you could make it work with references too... but that may get little to messy :)

dzenanz · 2020-12-10T14:51:13Z

@blowekamp you are free to take over this PR or create a new one. I believe you should have the right to push/amend this branch in my fork.

dzenanz

Looks great! Awesome that you added a specific test.

dzenanz · 2020-12-10T20:51:44Z

I can't formally approve my own PR, but I approve!

dzenanz · 2020-12-10T20:53:23Z

+  ImageType * image_rptr = image_ptr.GetPointer();
+  itk::WriteImage(image_rptr, "test1.mha");
+
+  ImageType * image_crptr = image_ptr.GetPointer();


Shouldn't here be a const ImageType * image_crptr = image_ptr.GetConstPointer(); or something else involving const qualifier?

N-Dekker · 2020-12-11T10:53:26Z

Thanks for addressing my comments about the new test that you added!

I think the test is still somewhat limited, as it only tests that it is possible to call WriteImage. It does not test the effect of calling the function! Ideally you would also test that WriteImage actually writes an image to a file, by checking if the image can be read back from the written file. But I think such an extra test could be added later, once we also have a simple itk::ReadImage function 😄 Pseudo code:

itk::WriteImage(image, fileName);
ASSERT_EQ(itk::ReadImage(file), image);

Anyway, that would be a nice future extension, in my opinion.

N-Dekker

I'm very glad that all (seven!) redundant WriteImage overloads can be removed this way 😄

blowekamp · 2020-12-11T13:13:04Z

@N-Dekker That you for your comments and suggestions. This problem seemed rather simple to begin with but with the requirements of implicit template parameters, and support for numerous input proved challenging due to implicit conversion not working with the implicit template parameters. What we have finally iterated towards is a new ( to me at least ), flexible and maintainable way to implement functions taking itk::Image pointers.

itk::WriteImage(image, fileName);
ASSERT_EQ(itk::ReadImage(file), image);

I don't believe that we have this functionality with ITK and the GTest framework and it would likely need some measures for tolerance for pixels and metadata.

blowekamp · 2020-12-11T14:33:15Z

I am seeing the following in testing errors:

/Users/runner/work/1/s/Modules/IO/ImageBase/test/itkWriteImageFunctionGTest.cxx:26: error: namespace is wrong

I am using an anonymous name space for file local class declarations. I guess this is no longer allowed. What is the preferred way?

dzenanz · 2020-12-11T14:37:20Z

What is the preferred way?

I don't know right off the bat. I would have to search, maybe see other GTests.

dzenanz

Approved

Reduce to only one signature and use meta-programming to always convert the input image type to SmartPointer<const T>, and ensure that the writer is instantiated with a non-constant image type.

N-Dekker · 2020-12-11T20:57:00Z

Why did you change class to struct?

hjmjohnson

Very Nice.

…mageFunction ENH: remove redundant WriteImage overloads

dzenanz mentioned this pull request Dec 9, 2020

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

Closed

hjmjohnson reviewed Dec 9, 2020

View reviewed changes

Comment thread Modules/IO/ImageBase/include/itkImageFileWriter.h Outdated

dzenanz force-pushed the writeImageFunction branch from 2896df7 to 6d68d72 Compare December 9, 2020 17:22

N-Dekker reviewed Dec 9, 2020

View reviewed changes

Comment thread Modules/IO/ImageBase/include/itkImageFileWriter.h Outdated

blowekamp force-pushed the writeImageFunction branch from 6d68d72 to 2e9ac8e Compare December 10, 2020 15:45