search

QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
240 stars 61 forks source link

ENH: Add template for input ImageType for itk2Dicom #507

Closed jadh4v closed 2 months ago

fedorov commented 3 months ago

@jadh4v: @michaelonken looked at this and noticed something that I did not think about.

I think it's fine as long as VectorImageAdapter (i.e. itk::VectorImageToImageAdaptor<short, 3U>) is a compatible to ShortImageType which I don't know. If VectorImageAdapter includes pixel data types that we cannot handle it should be checked somewhere in the code before starting conversions.

Can you please comment on this?

jadh4v commented 3 months ago

@jadh4v: @michaelonken looked at this and noticed something that I did not think about.

I think it's fine as long as VectorImageAdapter (i.e. itk::VectorImageToImageAdaptor<short, 3U>) is a compatible to ShortImageType which I don't know. If VectorImageAdapter includes pixel data types that we cannot handle it should be checked somewhere in the code before starting conversions.

Can you please comment on this?

Good catch. The use cases are limited by explicit instantiation of the templated function itkimage2dcmSegmentation. So we only expect two versions of the function to be used through the compiled library. However nothing stops someone from instantiating the function for other pixelTypes when building from source. We have two options:

  1. We could check for PixelType to be short during runtime and throw error that we don't handle the conversion.
  2. Write the templated function so that it only allows short pixeltype during compile time. Any attempt to instantiate the function with a non-short type will result in a compile time error.
jadh4v commented 3 months ago

I have pushed a commit adding the restriction to the template function. Trying to instantiate the function with say VectorImageToImageAdaptor<int, 3U> with int as PixelType will result in the following error:

error C3190: 'DcmDataset *dcmqi::Itk2DicomConverter::itkimage2dcmSegmentation(std::vector<DcmDataset *,std::allocator<DcmDataset *>>,std::vector<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>,std::allocator<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>>>,const std::string &,bool,bool,bool)'
with the provided template arguments is not the explicit instantiation of any member function of 'dcmqi::Itk2DicomConverter' 
dcmqi\libsrc\Itk2DicomConverter.cpp(713,44): error C2945: explicit instantiation does not refer to a template-class specialization
jadh4v commented 3 months ago

Hmmm will have to add fixes for Linux and macOS.

michaelonken commented 3 months ago

I have pushed a commit adding the restriction to the template function. Trying to instantiate the function with say VectorImageToImageAdaptor<int, 3U> with int as PixelType will result in the following error:

error C3190: 'DcmDataset *dcmqi::Itk2DicomConverter::itkimage2dcmSegmentation(std::vector<DcmDataset *,std::allocator<DcmDataset *>>,std::vector<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>,std::allocator<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>>>,const std::string &,bool,bool,bool)'
with the provided template arguments is not the explicit instantiation of any member function of 'dcmqi::Itk2DicomConverter' 
dcmqi\libsrc\Itk2DicomConverter.cpp(713,44): error C2945: explicit instantiation does not refer to a template-class specialization

:+1: I think that's the best solution to already reject wrong usage during compile time.

jadh4v commented 2 months ago

@michaelonken @fedorov Looks like the CI is passing with my last fix. Please approve and merge.

FYI: @jcfr @thewtex