Skip to content

ENH: add a convenience function WriteImage#2160

Merged
hjmjohnson merged 2 commits intoInsightSoftwareConsortium:masterfrom
dzenanz:writeImageFunction
Dec 9, 2020
Merged

ENH: add a convenience function WriteImage#2160
hjmjohnson merged 2 commits intoInsightSoftwareConsortium:masterfrom
dzenanz:writeImageFunction

Conversation

@dzenanz
Copy link
Copy Markdown
Member

@dzenanz dzenanz commented Dec 8, 2020

Inspired by #2102 and dzenanz/ITKMorphologicalContourInterpolation@a9ac128.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Reduced boilerplate code by calling the convenience function.
@dzenanz dzenanz force-pushed the writeImageFunction branch from 816d19b to fc3c8d1 Compare December 8, 2020 23:15
@hjmjohnson hjmjohnson merged commit 4e87206 into InsightSoftwareConsortium:master Dec 9, 2020
@blowekamp
Copy link
Copy Markdown
Member

Please consider placing these procedural methods into a sub-namespace of itk.

Also consider just using the std::string methods. C strings can implicitly be converted to std::string. So the raw C ptr is just for efficiency (?), but this a convince function for code that should not be time critical, so the burden of the extra methods out ways the potential enhancement.

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

It seems to me that the const char * overloads aren't necessary, as @blowekamp said.

@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Dec 9, 2020

Good point Brad! I will also wait for review from Niels. Hans merged it too fast.

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

So the raw C ptr is just for efficiency (?), but this a convince function for code that should not be time critical, so the burden of the extra methods out ways the potential enhancement.

I agree. By the way, if the performance penalty of creating an std::string would be a serious problem (not in this case, I think) C++17 introduces std::string_view, which is more lightweight than std::string. That's for later, possibly... 😃

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

Please consider placing these procedural methods into a sub-namespace of itk.

Of course, that is why I originally proposed defining the function as a static member function of ImageFileWriter<>. #2102 So that it wouldn't look like a function without a home 😺 But I'm not really opposed to having it as a free function at itk namespace level, if that's what you want. I have to say it is quite convenient to have it at itk namespace.

@hjmjohnson
Copy link
Copy Markdown
Member

Good point Brad! I will also wait for a review from Niels. Hans merged it too fast.

Sorry, I'm just trying to keep the forward momentum. This topic had many discussions and I thought it represented a good step forward.

BTW: I agree with the proposed changes to drop const char *. The std::string_view would be a great candidate for macro.

# a macro to take advantage of newer C++ standards when available
#if C++17 
   using ITK_STRING_VIEW=std::string_view
#else
   using ITK_STRING_VIEW=std::string
#endif

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

Technically I wouldn't think that an overload for non-const TImage * should be necessary, when there is already an overload for const TImage *. Did you encounter any case where the non-const TImage * overload appeared necessary?

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

It looks like the const SmartPointer<const TImage> overloads may also be removed, as the compiler may deduce the constness of TImage automatically for the SmartPointer<TImage> overload. (For example, TImage could be deduced to const itk::Image<int>.)

@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Dec 9, 2020

Without non-const overloads there are compile errors:

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 itk::ITK_STRING_VIEW &,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(260): 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 itk::ITK_STRING_VIEW &,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(249): 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 itk::ITK_STRING_VIEW &,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(260): 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 itk::ITK_STRING_VIEW &,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(249): 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 itk::ITK_STRING_VIEW &,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(260): 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 itk::ITK_STRING_VIEW &,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(249): note: see declaration of 'itk::WriteImage'

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Dec 9, 2020

Is itk::ITK_STRING_VIEW already there? How is it defined?

N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 16, 2020
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
InsightSoftwareConsortium#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: InsightSoftwareConsortium#2160
Commit: 60b0984

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 18, 2020
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
InsightSoftwareConsortium#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: InsightSoftwareConsortium#2160
Commit: 60b0984

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 19, 2020
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
InsightSoftwareConsortium#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: InsightSoftwareConsortium#2160
Commit: 60b0984

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 19, 2020
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
InsightSoftwareConsortium#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: InsightSoftwareConsortium#2160
Commit: 60b0984

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
hjmjohnson pushed a commit that referenced this pull request Dec 22, 2020
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: #2160
Commit: 60b0984

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request May 5, 2022
Follow-up to ITK pull request InsightSoftwareConsortium/ITK#2160 commit InsightSoftwareConsortium/ITK@60b0984 "ENH: add a convenience function WriteImage", merged on 9 December 2020, and included with ITK v5.2rc01.
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request May 5, 2022
Follow-up to ITK pull request InsightSoftwareConsortium/ITK#2160 commit InsightSoftwareConsortium/ITK@60b0984 "ENH: add a convenience function WriteImage", merged on 9 December 2020, and included with ITK v5.2rc01.
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Added `itk::ReadImage<TOutputImage>(const std::string & filename)`, to
allows simplifying the C++ code for reading an image.

Suggested by Dženan Zukić at
InsightSoftwareConsortium#2102 (comment)

Follow-up to:
"ENH: add a convenience function WriteImage"
Pull request: InsightSoftwareConsortium#2160
Commit: add59ae

Replaced manual creation of `ImageFileReader` objects in Common tests by
calls to the new `itk::ReadImage` function. Also removed related
redundant try-catch blocks from SymmetricSecondRankTensorImage tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants